refactor: unbundle algebra from ENormed*#28803
refactor: unbundle algebra from ENormed*#28803astrainfinita wants to merge 10 commits intoleanprover-community:masterfrom
ENormed*#28803Conversation
Comments from Original PR #23961This section contains 7 comment(s) from the original PR, excluding bot comments. @astrainfinita (2025-04-11 20:55 UTC): @grunweg (2025-04-12 08:21 UTC):
@grunweg (2025-04-29 16:43 UTC): @grunweg (2025-05-06 17:41 UTC): @jcommelin (2025-06-07 07:03 UTC):
This looks like a good change to me. @FR-vdash-bot could you please respond to the open questions? @grunweg (2025-08-23 09:33 UTC): @astrainfinita (2025-08-23 11:08 UTC): |
PR summary c3c8322bc4Import changes for modified filesNo significant changes to the import graph Import changes for all files
|
| Current number | Change | Type |
|---|---|---|
| 86 | 2 | disabled deprecation lints |
Current commit 893698ed8b
Reference commit c3c8322bc4
You can run this locally as
./scripts/technical-debt-metrics.sh pr_summary
- The
relativevalue is the weighted sum of the differences with weight given by the inverse of the current value of the statistic. - The
absolutevalue is therelativevalue divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).
|
Let me know once this PR passes CI, then I'll be happy to review it. |
|
!bench |
|
Here are the benchmark results for commit abfecc2. Benchmark Metric Change
=============================================================================================
+ ~Mathlib.Algebra.Module.ZLattice.Basic instructions -7.5%
+ ~Mathlib.Analysis.CStarAlgebra.CStarMatrix instructions -13.1%
+ ~Mathlib.Analysis.CStarAlgebra.ContinuousFunctionalCalculus.NonUnital instructions -5.8%
+ ~Mathlib.Analysis.Calculus.ContDiff.Basic instructions -4.0%
+ ~Mathlib.Analysis.Calculus.ContDiff.FTaylorSeries instructions -7.4%
+ ~Mathlib.Analysis.Calculus.FDeriv.Analytic instructions -5.5%
+ ~Mathlib.Analysis.Calculus.FDeriv.Mul instructions -3.5%
+ ~Mathlib.Analysis.Fourier.FourierTransformDeriv instructions -3.8%
+ ~Mathlib.Analysis.InnerProductSpace.Adjoint instructions -8.6%
+ ~Mathlib.Analysis.Normed.Lp.lpSpace instructions -19.6%
+ ~Mathlib.Analysis.Normed.Operator.LinearIsometry instructions -7.1%
+ ~Mathlib.Analysis.NormedSpace.BallAction instructions -11.4%
+ ~Mathlib.Analysis.NormedSpace.Multilinear.Basic instructions -7.7%
+ ~Mathlib.Analysis.NormedSpace.Multilinear.Curry instructions -7.0%
+ ~Mathlib.Analysis.NormedSpace.PiTensorProduct.InjectiveSeminorm instructions -14.4%
+ ~Mathlib.Analysis.Seminorm instructions -11.8%
+ ~Mathlib.Geometry.Manifold.MFDeriv.NormedSpace instructions -6.2%
+ ~Mathlib.Geometry.Manifold.Riemannian.Basic instructions -19.8%
+ ~Mathlib.Geometry.Manifold.VectorBundle.Hom instructions -9.6%
+ ~Mathlib.Geometry.Manifold.VectorBundle.Riemannian instructions -10.6%
+ ~Mathlib.LinearAlgebra.Matrix.SchurComplement instructions -16.4%
+ ~Mathlib.MeasureTheory.Function.Holder instructions -13.1%
+ ~Mathlib.MeasureTheory.Function.LpSpace.Basic instructions -10.4%
+ ~Mathlib.MeasureTheory.Function.SimpleFuncDenseLp instructions -17.7%
+ ~Mathlib.MeasureTheory.Group.FundamentalDomain instructions -16.7%
- ~Mathlib.MeasureTheory.Integral.IntervalIntegral.Basic instructions 24.8%
+ ~Mathlib.NumberTheory.NumberField.Ideal.Basic instructions -52.8%
+ ~Mathlib.NumberTheory.NumberField.Units.DirichletTheorem instructions -13.7%
+ ~Mathlib.Topology.Algebra.Module.FiniteDimension instructions -10.6%
+ ~Mathlib.Topology.VectorBundle.Hom instructions -8.9%
+ ~Mathlib.Topology.VectorBundle.Riemannian instructions -7.0% |
2 files, Instructions +5.0⬝10⁹
4 files, Instructions +2.0⬝10⁹
13 files, Instructions +1.0⬝10⁹
98 files, Instructions -2.0⬝10⁹
67 files, Instructions -3.0⬝10⁹
23 files, Instructions -4.0⬝10⁹
14 files, Instructions -5.0⬝10⁹
14 files, Instructions -6.0⬝10⁹
10 files, Instructions -7.0⬝10⁹
7 files, Instructions -8.0⬝10⁹
4 files, Instructions -9.0⬝10⁹
6 files, Instructions -10.0⬝10⁹
3 files, Instructions -11.0⬝10⁹
8 files, Instructions -12.0⬝10⁹
4 files, Instructions -13.0⬝10⁹
2 files, Instructions -15.0⬝10⁹
2 files, Instructions -16.0⬝10⁹
2 files, Instructions -18.0⬝10⁹
3 files, Instructions -19.0⬝10⁹
2 files, Instructions -20.0⬝10⁹
|
|
!bench |
|
The last commit is even a slight typeclass generalisation. Nice find; let me PR that separately (edit: #28813). |
|
Here are the benchmark results for commit 584bd7f. Benchmark Metric Change
=============================================================================================
+ ~Mathlib.Algebra.Module.ZLattice.Basic instructions -7.6%
+ ~Mathlib.Analysis.CStarAlgebra.CStarMatrix instructions -13.1%
+ ~Mathlib.Analysis.CStarAlgebra.ContinuousFunctionalCalculus.NonUnital instructions -5.8%
+ ~Mathlib.Analysis.Calculus.ContDiff.Basic instructions -4.0%
+ ~Mathlib.Analysis.Calculus.ContDiff.FTaylorSeries instructions -7.4%
+ ~Mathlib.Analysis.Calculus.FDeriv.Analytic instructions -5.6%
+ ~Mathlib.Analysis.Calculus.FDeriv.Mul instructions -3.5%
+ ~Mathlib.Analysis.Fourier.FourierTransformDeriv instructions -3.8%
+ ~Mathlib.Analysis.InnerProductSpace.Adjoint instructions -8.6%
+ ~Mathlib.Analysis.Normed.Lp.lpSpace instructions -19.6%
+ ~Mathlib.Analysis.Normed.Operator.LinearIsometry instructions -7.1%
+ ~Mathlib.Analysis.NormedSpace.BallAction instructions -11.5%
+ ~Mathlib.Analysis.NormedSpace.Multilinear.Basic instructions -7.7%
+ ~Mathlib.Analysis.NormedSpace.Multilinear.Curry instructions -6.9%
+ ~Mathlib.Analysis.NormedSpace.PiTensorProduct.InjectiveSeminorm instructions -14.4%
+ ~Mathlib.Analysis.Seminorm instructions -11.8%
+ ~Mathlib.Geometry.Manifold.MFDeriv.NormedSpace instructions -6.2%
+ ~Mathlib.Geometry.Manifold.Riemannian.Basic instructions -19.8%
+ ~Mathlib.Geometry.Manifold.VectorBundle.Hom instructions -9.5%
+ ~Mathlib.Geometry.Manifold.VectorBundle.Riemannian instructions -10.6%
+ ~Mathlib.LinearAlgebra.Matrix.SchurComplement instructions -16.5%
+ ~Mathlib.MeasureTheory.Function.Holder instructions -13.0%
+ ~Mathlib.MeasureTheory.Function.LpSpace.Basic instructions -10.5%
+ ~Mathlib.MeasureTheory.Function.SimpleFuncDenseLp instructions -17.7%
+ ~Mathlib.MeasureTheory.Group.FundamentalDomain instructions -16.7%
+ ~Mathlib.NumberTheory.NumberField.Ideal.Basic instructions -53.2%
+ ~Mathlib.NumberTheory.NumberField.Units.DirichletTheorem instructions -13.6%
+ ~Mathlib.Topology.Algebra.Module.FiniteDimension instructions -10.5%
+ ~Mathlib.Topology.VectorBundle.Hom instructions -8.8%
+ ~Mathlib.Topology.VectorBundle.Riemannian instructions -6.9% |
2 files, Instructions +5.0⬝10⁹
3 files, Instructions +2.0⬝10⁹
14 files, Instructions +1.0⬝10⁹
99 files, Instructions -2.0⬝10⁹
62 files, Instructions -3.0⬝10⁹
24 files, Instructions -4.0⬝10⁹
17 files, Instructions -5.0⬝10⁹
11 files, Instructions -6.0⬝10⁹
12 files, Instructions -7.0⬝10⁹
5 files, Instructions -8.0⬝10⁹
4 files, Instructions -9.0⬝10⁹
6 files, Instructions -10.0⬝10⁹
3 files, Instructions -11.0⬝10⁹
8 files, Instructions -12.0⬝10⁹
4 files, Instructions -13.0⬝10⁹
2 files, Instructions -15.0⬝10⁹
2 files, Instructions -16.0⬝10⁹
2 files, Instructions -18.0⬝10⁹
3 files, Instructions -19.0⬝10⁹
2 files, Instructions -20.0⬝10⁹
|
grunweg
left a comment
There was a problem hiding this comment.
Thanks!
maintainer merge
|
🚀 Pull request has been placed on the maintainer queue by grunweg. |
|
Could you summarize the main changes to the typeclasses in the PR commit? e.g.:
|
|
This looks fine to me and I'll happily merge as is, but is there also a plan for removing these classes completely, sticking to ENorm, and replacing the classes here with Prop-valued classes saying "the ENorm is compatible with the topology (i.e. continuous)" etc? |
|
Let's get this in once you fix up the PR description / commit message. Thanks! |
|
✌️ astrainfinita can now approve this pull request. To approve and merge a pull request, simply reply with |
…nprover-community#28813) Discovered in leanprover-community#28803. Transitively part of the Carleson project, as this is about the enorm classes. While this PR is basically performance-neutral; transplanting this to leanprover-community#28803 speeds up the file by 20 * 10⁹ instructions.
|
bors r+ |
Further speed up the search in the algebraic typeclass hierarchy by avoiding searching for `TopologicalSpace`. This PR continues the work from #23961. - Change `ESeminormed(Add)Monoid` and `ENormed(Add)Monoid` so they no longer carry algebraic data. - Deprecate `ESeminormed(Add)CommMonoid` and `ENormed(Add)CommMonoid` in favor of `ESeminormed(Add)Monoid` and `ENormed(Add)Monoid` with a commutative algebraic typeclass. |Old|New| |
|
Build failed: |
|
I am not convinced that we should merge this now. I agree that there is a performance improvement, but if we want to go this way we should probably do it for the other classes like NormedAddCommGroup and friends. I expect this will give much bigger performance improvement even, but at the expense of a reduced readability of typeclasses declarations. Until we have a way to shortcut variables declarations (something like |
|
There is a PR (depending on this, which would need a rebase) which does the same change for |
|
I'm not too psyched about the readability loss either, but having worked in differential geometry long enough, I guess I got used to this. This is a problem which needs a solution, but I'm not convinced waiting it out is it. |
|
We merged PRs unbundling order and algebra. This gave a crazy performance improvement, but still there was a long discussion and several experiments to see the best way to do it. I'm saying we should do the same here. (And to be fair I'm personally completely in favor of merging, but I do think this should be discussed very seriously beforehand) |
|
The design of |
I believe the point is this: even if in this case there is not too much fallout from the change, it would be undesirable to have some typeclasses unbundled and some bundled without some underlying ground rules, otherwise we end up baking in a bunch of design decisions into the library that few people understand and will be difficult to maintain. As Sébastien said, there is a way forward on this, but we need to decide on a bigger plan first. For the time being, I've started a Zulip thread. bors d- |
|
🚫 All delegations have been removed from this PR. To re-add a delegation, reply with |
…nprover-community#28813) Discovered in leanprover-community#28803. Transitively part of the Carleson project, as this is about the enorm classes. While this PR is basically performance-neutral; transplanting this to leanprover-community#28803 speeds up the file by 20 * 10⁹ instructions.
|
This pull request has conflicts, please merge |
Further speed up the search in the algebraic typeclass hierarchy by avoiding searching for
TopologicalSpace.This PR continues the work from #23961.
ESeminormed(Add)MonoidandENormed(Add)Monoidso they no longer carry algebraic data.ESeminormed(Add)CommMonoidandENormed(Add)CommMonoidin favor ofESeminormed(Add)MonoidandENormed(Add)Monoidwith a commutative algebraic typeclass.[ESeminormed(Add)(Comm)Monoid E][(Add)(Comm)Monoid E] [ESeminormed(Add)Monoid E][ENormed(Add)(Comm)Monoid][(Add)(Comm)Monoid E] [ENormed(Add)Monoid]See Zulip discussion