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] - chore(Algebra.Lie): do not always coerce a LieSubmodule to a Submodule to get a type #16509

Closed
wants to merge 5 commits into from

Conversation

mattrobball
Copy link
Collaborator

@mattrobball mattrobball commented Sep 5, 2024

We add

instance (priority := high) coeSort : CoeSort (LieSubmodule R L M) (Type w) where
  coe N := { x : M // x ∈ N }

to avoid the coercion to a Sort being { x : M // x ∈ N.toSubmodule } and instead be N itself.

This allows removal of all adapation_notes from leanprover/lean4#5020.

Ultimately the API here should be expanded and Coe (LieSubmodule R L M) (Submodule R M)
should be minimized or removed to avoid forcing Lean to unify LieSubmodule and Submodule unnecessarily.


Open in Gitpod

Copy link

github-actions bot commented Sep 5, 2024

PR summary c170d9258e

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Algebra.Lie.Subalgebra 1011 1224 +213 (+21.07%)
Import changes for all files
Files Import difference
Mathlib.Algebra.Lie.Subalgebra 213

Declarations diff

+ instUniqueBot
+ instance (L' : LieSubalgebra R L) [IsArtinian R L] : IsArtinian R L'
+ instance (priority := high) coeSort : CoeSort (LieSubmodule R L M) (Type w)
+ instance (priority := mid) coeSubmodule : CoeOut (LieSubmodule R L M) (Submodule R M)
+ instance : Unique {x // x ∈ (⊥ : LieIdeal R L)}
+ instance [IsArtinian R M] (N : LieSubmodule R L M) : IsArtinian R N
+ instance [IsNoetherian R M] (N : LieSubmodule R L M) : IsNoetherian R N
+ instance [NoZeroSMulDivisors R M] : NoZeroSMulDivisors R N
- coeSubmodule
- instCanLiftSubmoduleLieSubmodule
- instance : Module R N
- instance {S : Type*} [Semiring S] [SMul S R] [SMul Sᵐᵒᵖ R] [Module S M] [Module Sᵐᵒᵖ M]
- module'

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 script/declarations_diff.sh contains some details about this script.

@jcommelin jcommelin requested a review from ocfnash September 5, 2024 14:34
@ocfnash ocfnash self-assigned this Sep 6, 2024
@mattrobball
Copy link
Collaborator Author

!bench

Mathlib/Algebra/Lie/Semisimple/Basic.lean Outdated Show resolved Hide resolved
@@ -107,6 +107,9 @@ instance [SMul R₁ R] [Module R₁ L] [IsScalarTower R₁ R L] (L' : LieSubalge
instance (L' : LieSubalgebra R L) [IsNoetherian R L] : IsNoetherian R L' :=
isNoetherian_submodule' _

instance (L' : LieSubalgebra R L) [IsArtinian R L] : IsArtinian R L' :=
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could probably be moved downstream to avoid additional imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed but I'm fine with the import bump if you are.

Mathlib/Algebra/Lie/Submodule.lean Outdated Show resolved Hide resolved
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 0c698ba.
There were significant changes against commit 82e0419:

  Benchmark                             Metric         Change
  ===========================================================
- ~Mathlib.Algebra.Lie.Abelian          instructions    49.8%
- ~Mathlib.Algebra.Lie.Submodule        instructions    17.4%
- ~Mathlib.Algebra.Lie.TensorProduct    instructions    23.0%
- ~Mathlib.Algebra.Lie.TraceForm        instructions    12.1%
- ~Mathlib.Algebra.Lie.Weights.Basic    instructions    20.8%
- ~Mathlib.Algebra.Lie.Weights.Cartan   instructions    25.4%
- ~Mathlib.Algebra.Lie.Weights.Chain    instructions    18.6%
- ~Mathlib.Algebra.Lie.Weights.Linear   instructions    41.8%

@mattrobball
Copy link
Collaborator Author

I suspect this will reverse if the coercion from LieSubmodule to Submodule is removed (or minimized) but that is a bit of work.

@ocfnash
Copy link
Contributor

ocfnash commented Sep 10, 2024

I haven't forgotten about this PR, I'm just very busy but I will get to it soon.

Copy link
Contributor

@ocfnash ocfnash left a comment

Choose a reason for hiding this comment

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

Thanks for all this work.

bors d+

Mathlib/Algebra/Lie/Weights/Chain.lean Outdated Show resolved Hide resolved
@@ -107,6 +107,9 @@ instance [SMul R₁ R] [Module R₁ L] [IsScalarTower R₁ R L] (L' : LieSubalge
instance (L' : LieSubalgebra R L) [IsNoetherian R L] : IsNoetherian R L' :=
isNoetherian_submodule' _

instance (L' : LieSubalgebra R L) [IsArtinian R L] : IsArtinian R L' :=
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed but I'm fine with the import bump if you are.

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Sep 11, 2024

✌️ 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.

@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 12, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Sep 24, 2024
@mattrobball
Copy link
Collaborator Author

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Sep 24, 2024
mathlib-bors bot pushed a commit that referenced this pull request Sep 24, 2024
…dule` to get a type (#16509)

We add
```lean
instance (priority := high) coeSort : CoeSort (LieSubmodule R L M) (Type w) where
  coe N := { x : M // x ∈ N }
```
to avoid the coercion to a `Sort` being `{ x : M // x ∈ N.toSubmodule }` and instead be `N` itself.

This allows removal of all `adapation_note`s from leanprover/lean4#5020.

Ultimately the API here should be expanded and `Coe (LieSubmodule R L M) (Submodule R M)`
should be minimized or removed to avoid forcing Lean to unify `LieSubmodule` and `Submodule` unnecessarily.



Co-authored-by: Matthew Robert Ballard <[email protected]>
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Sep 24, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore(Algebra.Lie): do not always coerce a LieSubmodule to a Submodule to get a type [Merged by Bors] - chore(Algebra.Lie): do not always coerce a LieSubmodule to a Submodule to get a type Sep 24, 2024
@mathlib-bors mathlib-bors bot closed this Sep 24, 2024
@mathlib-bors mathlib-bors bot deleted the mrb/lie_coe branch September 24, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants