-
Notifications
You must be signed in to change notification settings - Fork 369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Merged by Bors] - chore(Algebra/Algebra/Defs): add an algebraMap
field to Algebra
instead of extending RingHom
#20518
Conversation
PR summary 118b2fe326Import changes for modified filesNo significant changes to the import graph Import changes for all files
Declarations diff
You can run this locally as follows## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>
## more verbose report:
./scripts/declarations_diff.sh long <optional_commit> The doc-module for Decrease in tech debt: (relative, absolute) = (1.00, 0.00)
Current commit 118b2fe326 You can run this locally as
|
!bench |
Here are the benchmark results for commit 2890b28. |
!bench |
Here are the benchmark results for commit e85e968. |
!bench |
Here are the benchmark results for commit 6fb5948. |
!bench |
Here are the benchmark results for commit 6fb5948. |
I should say that I have no particular suspicion that mathlib will compile more quickly or more slowly, but I thought it was worth benchmarking giving that we're changing such a fundamental definition and that typeclass inference can sometimes be very delicate. I hadn't realised that there were problems with the benchmarking bot. |
!bench |
Here are the benchmark results for commit 02cc50f. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maintainer delegate
I am not an expert in the finer parts of typeclass inference so I'd like another maintainer to sign off on this, but it's great to see that this change decreases build time a little and the diff looks fine to me (github is making much more of a meal of it than it could do).
@@ -64,7 +64,7 @@ attribute [simp] algebraMap_σ | |||
noncomputable instance {R₀} [CommRing R₀] [Algebra R₀ R] [Algebra R₀ S] [IsScalarTower R₀ R S] : | |||
Algebra R₀ P.Ring where | |||
__ := Module.compHom P.Ring (algebraMap R₀ R) | |||
__ := (algebraMap R P.Ring).comp (algebraMap R₀ R) | |||
algebraMap := (algebraMap R P.Ring).comp (algebraMap R₀ R) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Module.compHom
thing on line 66 still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is. There's Algebra.compHom
that can replace the whole def now but this is probably not in scope of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR at #20708
🚀 Pull request has been placed on the maintainer queue by kbuzzard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors d+
✌️ edegeltje can now approve this pull request. To approve and merge a pull request, simply reply with |
bors r+ |
bors r+ |
Already running a review |
…nstead of extending `RingHom` (#20518) as proposed on [zulip](https://leanprover.zulipchat.com/#narrow/channel/287929-mathlib4/topic/too.20many.20.60out.60s/near/492053667)
Pull request successfully merged into master. Build succeeded: |
algebraMap
field to Algebra
instead of extending RingHom
algebraMap
field to Algebra
instead of extending RingHom
as proposed on zulip