Skip to content

[Merged by Bors] - perf (Data.Rat): reduce algebra instances for Rat#6282

Closed
mattrobball wants to merge 3 commits intomasterfrom
mrb/rat_field_inst
Closed

[Merged by Bors] - perf (Data.Rat): reduce algebra instances for Rat#6282
mattrobball wants to merge 3 commits intomasterfrom
mrb/rat_field_inst

Conversation

@mattrobball
Copy link
Copy Markdown
Contributor

@mattrobball mattrobball commented Aug 1, 2023

This removes the with pattern from the Field and CommSemiGroupWithZero instances on Rat.

The resulting term decreases in size by an order of magnitude (at least).


Open in Gitpod

@mattrobball
Copy link
Copy Markdown
Contributor Author

!bench

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit 7a00598.
There were significant changes against commit 358bd33:

  Benchmark                                    Metric         Change
  ==================================================================
+ ~Mathlib.Data.Rat.Basic                      instructions   -15.3%
+ ~Mathlib.NumberTheory.Padics.PadicIntegers   instructions    -7.3%

@mattrobball
Copy link
Copy Markdown
Contributor Author

mattrobball commented Aug 1, 2023

Currently there are commented-out sanity test rfl's to make sure no diamonds appeared in the modification of the instances.

My suggestion: the reviewer should pull a copy, un-comment, and check the two files compile. Then if they are happy, tell me to delete them.

Very much open to other suggestions.

@eric-wieser
Copy link
Copy Markdown
Member

Removing those comments seems fine to me. I think in the long run we should have a linter to catch diamonds here.

@eric-wieser
Copy link
Copy Markdown
Member

bors d+

@bors
Copy link
Copy Markdown

bors bot commented Aug 2, 2023

✌️ mattrobball can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot added delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). and removed awaiting-review labels Aug 2, 2023
@mattrobball
Copy link
Copy Markdown
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Aug 2, 2023
This removes the `with` pattern from the `Field` and `CommSemiGroupWithZero` instances on `Rat`.

The resulting term decreases in size by an order of magnitude (at least).



Co-authored-by: Matthew Robert Ballard <100034030+mattrobball@users.noreply.github.com>
@bors
Copy link
Copy Markdown

bors bot commented Aug 2, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title perf (Data.Rat): reduce algebra instances for Rat [Merged by Bors] - perf (Data.Rat): reduce algebra instances for Rat Aug 2, 2023
@bors bors bot closed this Aug 2, 2023
@bors bors bot deleted the mrb/rat_field_inst branch August 2, 2023 20:53
kim-em pushed a commit that referenced this pull request Aug 3, 2023
This removes the `with` pattern from the `Field` and `CommSemiGroupWithZero` instances on `Rat`.

The resulting term decreases in size by an order of magnitude (at least).



Co-authored-by: Matthew Robert Ballard <100034030+mattrobball@users.noreply.github.com>
kim-em pushed a commit that referenced this pull request Aug 3, 2023
This removes the `with` pattern from the `Field` and `CommSemiGroupWithZero` instances on `Rat`.

The resulting term decreases in size by an order of magnitude (at least).



Co-authored-by: Matthew Robert Ballard <100034030+mattrobball@users.noreply.github.com>
kim-em pushed a commit that referenced this pull request Aug 14, 2023
This removes the `with` pattern from the `Field` and `CommSemiGroupWithZero` instances on `Rat`.

The resulting term decreases in size by an order of magnitude (at least).



Co-authored-by: Matthew Robert Ballard <100034030+mattrobball@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants