-
Notifications
You must be signed in to change notification settings - Fork 399
feat: generalize Mathlib.Analysis #23189
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
Conversation
This is one of a series of PRs that generalizes type classes across Mathlib. These are generated using a new linter that tries to re-elaborate theorem definitions with more general type classes to see if it succeeds. It will accept the generalization if deleting the entire type class causes the theorem to fail to compile, and the old type class can not simply be re-synthesized with the new declaration. Otherwise, the generalization is rejected as the type class is not being generalized, but can simply be replaced by implicit type class synthesis or an implicit type class in a variable block being pulled in. The linter currently output debug statements indicating source file positions where type classes should be generalized, and a script then makes those edits. This file contains a subset of those generalizations. The linter and the script performing re-writes is available in commit 5e2b704. Also see discussion on Zulip here: https://leanprover.zulipchat.com/#narrow/channel/270676-lean4/topic/Elab.20to.20generalize.20type.20classes.20for.20theorems/near/498862988 https://leanprover.zulipchat.com/#narrow/channel/287929-mathlib4/topic/Elab.20to.20generalize.20type.20classes.20for.20theorems/near/501288855
!bench |
PR summary 1c2912c3baImport changes for modified filesNo significant changes to the import graph Import changes for all files
Declarations diffNo declarations were harmed in the making of this PR! 🐙 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 No changes to technical debt.You can run this locally as
|
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 stopped reviewing at a certain point. I think some of these are okay, but there are quite a few that I think are not doing anything useful.
Moreover, something about this linter makes me skeptical: it only caught declarations with type class arguments provided explicitly in the declaration (as opposed to in a variable
line). This makes me suspect that it didn't look for changes to other declarations (or, if it did, it tried to change variable
blocks which failed because some of the declarations needed the existing variables). In either of these cases, I think we shouldn't merge these because it is introducing semi-arbitrary discrepancies in generalization.
theorem IsLittleOTVS.add [IsTopologicalAddGroup E] [ContinuousSMul 𝕜 E] | ||
theorem IsLittleOTVS.add [ContinuousAdd E] [ContinuousSMul 𝕜 E] |
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.
This is one of those spurious generalizations. A group with ContinuousAdd
and ContinuouSMul
is a IsTopologicalAddGroup
.
theorem EuclideanSpace.nnnorm_eq {𝕜 : Type*} [RCLike 𝕜] {n : Type*} [Fintype n] | ||
theorem EuclideanSpace.nnnorm_eq {𝕜 : Type*} [NormedRing 𝕜] {n : Type*} [Fintype n] |
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.
Here and the ones below: EuclideanSpace
is only intended to be used for RCLike 𝕜
. If someone wants this for a NormedRing 𝕜
, then they should use PiLp 𝕜 (n → 𝕜)
(or whatever is the correct syntax).
[AddCommGroup E] [Module 𝕜 E] [TopologicalSpace E] {S : Set E} : | ||
[Zero E] [MulAction 𝕜 E] [TopologicalSpace E] {S : Set E} : |
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 think this is spurious. IsVonNBounded
is only relevant in topological vector spaces.
Here are the benchmark results for commit 1c2912c. |
|
Closing based on feedback (e.g. see https://leanprover.zulipchat.com/#narrow/channel/144837-PR-reviews/topic/.2322337.20type.20class.20generalization/with/507342720) |
This is one of a series of PRs that generalizes type classes across Mathlib. These are generated using a new linter that tries to re-elaborate theorem definitions with more general type classes to see if it succeeds. It will accept the generalization if deleting the entire type class causes the theorem to fail to compile, and the old type class can not simply be re-synthesized with the new declaration. Otherwise, the generalization is rejected as the type class is not being generalized, but can simply be replaced by implicit type class synthesis or an implicit type class in a variable block being pulled in.
The linter currently output debug statements indicating source file positions where type classes should be generalized, and a script then makes those edits. This file contains a subset of those generalizations. The linter and the script performing re-writes is available in commit 5e2b704.
Also see discussion on Zulip here:
https://leanprover.zulipchat.com/#narrow/channel/270676-lean4/topic/Elab.20to.20generalize.20type.20classes.20for.20theorems/near/498862988 https://leanprover.zulipchat.com/#narrow/channel/287929-mathlib4/topic/Elab.20to.20generalize.20type.20classes.20for.20theorems/near/501288855