Skip to content
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] - fix: change instance priorities in algebra hierarchy #5941

Closed
wants to merge 7 commits into from

Conversation

kbuzzard
Copy link
Member

@kbuzzard kbuzzard added the WIP Work in progress label Jul 16, 2023
@kbuzzard
Copy link
Member Author

!bench

@eric-wieser
Copy link
Member

I'm glad to see I'm not the only one who consistently mispells "hierarchy"...

@eric-wieser eric-wieser changed the title fix: change instance priorities in algebra heirarchy fix: change instance priorities in algebra hierarchy Jul 16, 2023
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 4c08f67.
There were significant changes against commit f1fbf07:

  Benchmark                                         Metric         Change
  =======================================================================
+ build                                             maxrss         -28.8%
- ~Mathlib.Algebra.DirectSum.Module                 instructions     6.5%
+ ~Mathlib.Algebra.Lie.DirectSum                    instructions    -5.2%
+ ~Mathlib.Analysis.Analytic.Linear                 instructions   -19.5%
+ ~Mathlib.Analysis.InnerProductSpace.Adjoint       instructions   -31.2%
+ ~Mathlib.Analysis.NormedSpace.Multilinear         instructions    -7.1%
+ ~Mathlib.Analysis.NormedSpace.OperatorNorm        instructions    -6.1%
+ ~Mathlib.CategoryTheory.Sites.Sheaf               instructions    -5.9%
- ~Mathlib.Data.Polynomial.Monomial                 instructions     5.4%
+ ~Mathlib.FieldTheory.IsAlgClosed.Basic            instructions   -10.5%
- ~Mathlib.LinearAlgebra.DirectSum.Finsupp          instructions    25.3%
- ~Mathlib.LinearAlgebra.DirectSum.TensorProduct    instructions     8.4%
+ ~Mathlib.NumberTheory.Cyclotomic.PrimitiveRoots   instructions    -8.1%
+ ~Mathlib.RingTheory.Adjoin.Field                  instructions   -30.9%
+ ~Mathlib.RingTheory.AlgebraicIndependent          instructions   -18.3%
+ ~Mathlib.RingTheory.Norm                          instructions   -10.6%
+ ~Mathlib.Topology.Instances.Complex               instructions    -6.6%

structure AC extends A
structure ABC extends AB, AC
then AB.toA should have higher priority than AC.toA
Todo: write linter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done automatically? I mean, when declaring a new class, put priority 200 on classes it extends with a toA, priority 100 otherwise (and maybe priority 1000 if it is a specific instance that doesn't always apply, like we did in mathlib3)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth opening a lean4 issue about this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's supposed to happen here?

structure AB extends A
structure AC extends A
structure ABC extends AB, AC
structure ACB extends AC. AB

Do we need a weight-style system like Coq, where the priority is the sum of all the paths, instead of a global priority on each step?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened leanprover/lean4#2325

@eric-wieser: just warn the user that they do something stupid, and ask them to reorder the extended structures. The sum of priorities is not good: you want to make sure that all the highest priority paths only use the "correct" projections.

Copy link
Member

@eric-wieser eric-wieser Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that this effectively forces us to choose a global partial order over all typeclasses that exist; potentially something as silly as "the extends clause must be in alphabetical order"...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LeftCancelMonoid, RightCancelMonoid and CommMonoid all extend Monoid, and if we declare commutativity to be our favourite path (it's surely the most common path) then CancelMonoid, which extends LeftCancelMonoid and RightCancelMonoid, is left with no "correct" projection.

@fpvandoorn fpvandoorn added awaiting-review awaiting-CI and removed WIP Work in progress labels Jul 17, 2023
@fpvandoorn
Copy link
Member

I think we can merge this without my comment, since it already saves a lot of memory usage. We can fix more instance priorities systematically in later PRs.

@fpvandoorn
Copy link
Member

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Jul 17, 2023
bors bot pushed a commit that referenced this pull request Jul 17, 2023
@bors
Copy link

bors bot commented Jul 17, 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 fix: change instance priorities in algebra hierarchy [Merged by Bors] - fix: change instance priorities in algebra hierarchy Jul 17, 2023
@bors bors bot closed this Jul 17, 2023
@bors bors bot deleted the instance_prio branch July 17, 2023 16:37
semorrison pushed a commit that referenced this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants