Skip to content

[Merged by Bors] - feat(Algebra/Pi): add various AlgEquivs and RingEquivs#24520

Closed
chrisflav wants to merge 12 commits intomasterfrom
chrisflav-picongr
Closed

[Merged by Bors] - feat(Algebra/Pi): add various AlgEquivs and RingEquivs#24520
chrisflav wants to merge 12 commits intomasterfrom
chrisflav-picongr

Conversation

@chrisflav
Copy link
Copy Markdown
Member

@chrisflav chrisflav commented May 1, 2025

Add

  • RingEquiv.sumArrowEquivProdArrow: ring version of Equiv.sumArrowEquivProdArrow.
  • AlgEquiv.sumArrowEquivProdArrow: algebra version of Equiv.sumArrowEquivProdArrow.
  • AlgEquiv.piCongrLeft': algebra version of Equiv.piCongrLeft.
  • AlgEquiv.piCongrLeft: algebra version of Equiv.piCongrLeft.
  • AlgEquiv.funUnique: algebra version of Equiv.funUnique.
  • AlgEquiv.prodCongr: algebra version of Equiv.prodCongr.

The four last ones already have ring versions.


Open in Gitpod

@chrisflav chrisflav added the t-algebra Algebra (groups, rings, fields, etc) label May 1, 2025
@github-actions github-actions bot added the large-import Automatically added label for PRs with a significant increase in transitive imports label May 1, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 1, 2025

PR summary 824629269a

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Algebra.Algebra.Pi 706 707 +1 (+0.14%)
Import changes for all files
Files Import difference
3 files Mathlib.Algebra.Algebra.Pi Mathlib.RingTheory.Localization.Away.Basic Mathlib.RingTheory.Localization.Pi
1

Declarations diff

+ funUnique
+ funUnique_apply
+ funUnique_symm_apply
+ piCongrLeft
+ piCongrLeft'
+ piCongrLeft'_apply
+ piCongrLeft'_symm_apply
+ piCongrLeft_apply
+ piCongrLeft_symm_apply
+ prodCongr
+ prodCongr_apply
+ prodCongr_symm_apply
+ sumArrowEquivProdArrow_symm_apply
+ sumArrowEquivProdArrow_symm_apply_inr
++ sumArrowEquivProdArrow
++ sumArrowEquivProdArrow_apply

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.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@github-actions github-actions bot removed the large-import Automatically added label for PRs with a significant increase in transitive imports label May 1, 2025
@Ruben-VandeVelde
Copy link
Copy Markdown
Contributor

Looking at the stats, I'm afraid I'm not really convinced by the import changes here, and even less convinced that they should happen in this PR

@chrisflav
Copy link
Copy Markdown
Member Author

Looking at the stats, I'm afraid I'm not really convinced by the import changes here, and even less convinced that they should happen in this PR

I am not sure I understand: What do you suggest? Revert the last three commits and accept the +19% import bump? Or just split this PR?

@eric-wieser
Copy link
Copy Markdown
Member

My vote would be to revert the move and accept the import shuffe; the import changes shallow out pretty quickly

@mistarro
Copy link
Copy Markdown
Collaborator

So is the inconsistency with having Ring.sumArrowEquivProdArrow in Mathlib.Algebra.Ring.Equiv and Algebra.sumArrowEquivProdArrow is in Mathlib.Algebra.Algebra.Pi ok? Maybe it might be moved the other way round?

Comment on lines +903 to +909
@[simp]
lemma sumArrowEquivProdArrow_symm_apply_inl (α β) (x : (α → R) × (β → R)) (a : α) :
(sumArrowEquivProdArrow R α β).symm x (Sum.inl a) = x.fst a := rfl

@[simp]
lemma sumArrowEquivProdArrow_symm_apply_inr (α β) (x : (α → R) × (β → R)) (b : β) :
(sumArrowEquivProdArrow R α β).symm x (Sum.inr b) = x.snd b := rfl
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think these should say

Suggested change
@[simp]
lemma sumArrowEquivProdArrow_symm_apply_inl (α β) (x : (α → R) × (β → R)) (a : α) :
(sumArrowEquivProdArrow R α β).symm x (Sum.inl a) = x.fst a := rfl
@[simp]
lemma sumArrowEquivProdArrow_symm_apply_inr (α β) (x : (α → R) × (β → R)) (b : β) :
(sumArrowEquivProdArrow R α β).symm x (Sum.inr b) = x.snd b := rfl
@[simp]
lemma sumArrowEquivProdArrow_apply (α β) (x) :
sumArrowEquivProdArrow R α β x = Equiv.sumArrowEquivProdArrow x := rfl
@[simp]
lemma sumArrowEquivProdArrow_symm_apply (α β) (x : (α → R) × (β → R)) (b : β) :
(sumArrowEquivProdArrow R α β).symm x = (Equiv.sumArrowEquivProdArrow).symm x := rfl

which should replace the simps above (unless it can generate exactly these for you)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(same for the other similar results)

Copy link
Copy Markdown
Member Author

@chrisflav chrisflav May 24, 2025

Choose a reason for hiding this comment

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

Don't you then lose all ring hom simp lemmas on RingEquiv.sumArrowEquivProdArrow? Or do you suggest to add map_{add,mul,zero,one} lemmas for Equiv.sumArrowEquivProdArrow?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Those lemmas would apply before this one I think.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It does not seem so:

example (x y)  : sumArrowEquivProdArrow α β R (x + y) =
    sumArrowEquivProdArrow α β R x + sumArrowEquivProdArrow α β R y := by
  simp

fails (and succeeds if I remove simp from the lemmas you suggest).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it work if you use simp 900 on the lemmas I suggested?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, it only starts working with simp 100 (simp 101 is the first non-working one).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Strange, thanks for investigating! I can look into this further tomorrow

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#25165 will help here I think. For now, can you set the priority to low - 1, with a comment saying that this is lower than map_add etc.? Then we can merge this without the other one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since #25165 was merged before this one, I switched all the low - 1 to low.

@eric-wieser
Copy link
Copy Markdown
Member

Could you update the description to be explicit about what the new defs are, and how they relate to existing ones?

@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes. label May 24, 2025
@chrisflav chrisflav changed the title feat(Algebra/Pi): add AlgEquiv.prodCongr feat(Algebra/Pi): add various AlgEquivs and RingEquivs May 24, 2025
@chrisflav chrisflav removed the awaiting-author A reviewer has asked the author a question or requested changes. label May 24, 2025
@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes. label May 24, 2025
@chrisflav chrisflav added awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. and removed awaiting-author A reviewer has asked the author a question or requested changes. labels May 24, 2025
@github-actions github-actions bot removed the awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. label May 24, 2025
@chrisflav chrisflav requested a review from eric-wieser June 9, 2025 09:20
Copy link
Copy Markdown
Member

@erdOne erdOne left a comment

Choose a reason for hiding this comment

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

The discussion above seems to be addressed already, and the PR has been stale for months so I'll

maintainer merge

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 4, 2025

🚀 Pull request has been placed on the maintainer queue by erdOne.

@ghost ghost added the maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. label Jul 4, 2025
@riccardobrasca
Copy link
Copy Markdown
Member

Thanks!

bors merge

@ghost ghost added ready-to-merge This PR has been sent to bors. and removed maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. labels Jul 5, 2025
mathlib-bors bot pushed a commit that referenced this pull request Jul 5, 2025
Add

- `RingEquiv.sumArrowEquivProdArrow`: ring version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.sumArrowEquivProdArrow`: algebra version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.piCongrLeft'`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.piCongrLeft`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.funUnique`: algebra version of `Equiv.funUnique`.
- `AlgEquiv.prodCongr`: algebra version of `Equiv.prodCongr`.

The four last ones already have ring versions.
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Jul 5, 2025

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(Algebra/Pi): add various AlgEquivs and RingEquivs [Merged by Bors] - feat(Algebra/Pi): add various AlgEquivs and RingEquivs Jul 5, 2025
@mathlib-bors mathlib-bors bot closed this Jul 5, 2025
@mathlib-bors mathlib-bors bot deleted the chrisflav-picongr branch July 5, 2025 05:52
joelriou pushed a commit to joelriou/mathlib4 that referenced this pull request Jul 7, 2025
…r-community#24520)

Add

- `RingEquiv.sumArrowEquivProdArrow`: ring version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.sumArrowEquivProdArrow`: algebra version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.piCongrLeft'`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.piCongrLeft`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.funUnique`: algebra version of `Equiv.funUnique`.
- `AlgEquiv.prodCongr`: algebra version of `Equiv.prodCongr`.

The four last ones already have ring versions.
callesonne pushed a commit to callesonne/mathlib4 that referenced this pull request Jul 24, 2025
…r-community#24520)

Add

- `RingEquiv.sumArrowEquivProdArrow`: ring version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.sumArrowEquivProdArrow`: algebra version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.piCongrLeft'`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.piCongrLeft`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.funUnique`: algebra version of `Equiv.funUnique`.
- `AlgEquiv.prodCongr`: algebra version of `Equiv.prodCongr`.

The four last ones already have ring versions.
hrmacbeth pushed a commit to szqzs/mathlib4 that referenced this pull request Jul 28, 2025
…r-community#24520)

Add

- `RingEquiv.sumArrowEquivProdArrow`: ring version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.sumArrowEquivProdArrow`: algebra version of `Equiv.sumArrowEquivProdArrow`.
- `AlgEquiv.piCongrLeft'`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.piCongrLeft`: algebra version of `Equiv.piCongrLeft`.
- `AlgEquiv.funUnique`: algebra version of `Equiv.funUnique`.
- `AlgEquiv.prodCongr`: algebra version of `Equiv.prodCongr`.

The four last ones already have ring versions.
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. t-algebra Algebra (groups, rings, fields, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants