Skip to content

chore: get rid of generic hom coercions#21031

Open
YaelDillies wants to merge 1 commit intomasterfrom
no_generic_hom_coe
Open

chore: get rid of generic hom coercions#21031
YaelDillies wants to merge 1 commit intomasterfrom
no_generic_hom_coe

Conversation

@YaelDillies
Copy link
Copy Markdown
Contributor


Open in Gitpod

@YaelDillies YaelDillies added the WIP Work in progress label Jan 24, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 24, 2025

PR summary 58f4dfcb3c

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Algebra.Group.Hom.Defs 111 112 +1 (+0.90%)
Mathlib.Algebra.GroupWithZero.Hom 124 125 +1 (+0.81%)
Mathlib.Algebra.Group.Equiv.Defs 126 127 +1 (+0.79%)
Mathlib.Algebra.Group.Units.Hom 138 139 +1 (+0.72%)
Import changes for all files
Files Import difference
15 files Mathlib.Algebra.Free Mathlib.Algebra.Group.Commute.Hom Mathlib.Algebra.Group.Equiv.Defs Mathlib.Algebra.Group.Ext Mathlib.Algebra.Group.Hom.Basic Mathlib.Algebra.Group.Hom.CompTypeclasses Mathlib.Algebra.Group.Hom.Defs Mathlib.Algebra.Group.Idempotent Mathlib.Algebra.Group.Invertible.Basic Mathlib.Algebra.Group.ModEq Mathlib.Algebra.Group.TypeTags.Hom Mathlib.Algebra.Group.Units.Hom Mathlib.Algebra.GroupWithZero.Equiv Mathlib.Algebra.GroupWithZero.Hom Mathlib.Algebra.GroupWithZero.Idempotent
1

Declarations diff

+ MonoidHom.ofClass
+ MulEquiv.ofClass
+ MulEquiv.ofClass_injective
+ MulHom.coe_ofClass
+ MulHom.ofClass
+ OneHom.coe_ofClass
+ OneHom.ofClass
+ apply_ofClass_symm_apply
+ coe_symm_ofClass
+ coe_toMulEquiv
+ instFunLike
+ instance : Coe (M ≃* N) (M →* N) := ⟨toMonoidHom⟩
+ instance : Coe (α →*o β) (α →* β) := ⟨toMonoidHom⟩
+ instance : Coe (α →ₙ+* β) (α →+ β) := ⟨toAddMonoidHom⟩
+ instance : CoeOut (r ≃r s) (α ≃ β) := ⟨toEquiv⟩
+ isLocalHom_monoidHomOfClass
+ isLocalRingHom_toMonoidHom
+ mapEquiv
+ map_id
+ map_map
+ ofClass_symm
+ ofClass_symm_apply_apply
+ ofClass_symm_comp_self
+ refl_toMonoidHom
+ self_comp_ofClass_symm
+ symm_ofClass
+ symm_toEquiv
+ toEquiv_eq_ofClass
+ toMonoidHom_comp_toMonoidHom_symm
+ toMonoidHom_eq_ofClass
+ toMonoidHom_symm_comp_toMonoidHom
+ toMulHom_eq_ofClass
+ trans_toMonoidHom
++ coe_ofClass
++ ofClass
++ symm_toOrderIso
- EquivLike.toEquiv
- MonoidHom.coe_coe
- MonoidHomClass.toMonoidHom
- MulEquivClass.toMulEquiv_injective
- MulHom.coe_coe
- MulHomClass.toMulHom
- OneHom.coe_coe
- OneHomClass.toOneHom
- RingHomClass.toRingHom
- _root_.EquivLike.apply_coe_symm_apply
- _root_.EquivLike.coe_coe
- _root_.EquivLike.coe_symm_apply_apply
- _root_.EquivLike.coe_symm_comp_self
- _root_.EquivLike.self_comp_coe_symm
- coe_monoidHom_comp_coe_monoidHom_symm
- coe_monoidHom_refl
- coe_monoidHom_symm_comp_coe_monoidHom
- coe_monoidHom_trans
- instance : CoeTC F (α →+* β)
- instance [MonoidHomClass F M N] : CoeTC F (M →* N)
- instance [MulHomClass F M N] : CoeTC F (M →ₙ* N)
- instance [OneHomClass F M N] : CoeTC F (OneHom M N)
- instance {F} [EquivLike F α β] : CoeTC F (α ≃ β)
- isLocalHom_toMonoidHom
- map_apply
- toEquiv_eq_coe
- toMulHom_eq_coe
-- coe_toEquiv_symm
-- toEquiv_symm
-- toMonoidHom_eq_coe

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).

@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 Jan 24, 2025
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 25, 2025
Copy link
Copy Markdown
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Overall I agree that the generic hom coercions didn't turn out to be particularly useful, so I would be in favour of the PR.

Could you also check that the documentation on how to set up FunLikes is appropriately updated?

section coe

@[to_additive]
instance : EquivLike (M ≃* N) M N where
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't this section double with Group/Equiv/Defs.lean?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Very likely. I seem to have botched a rebase


/-- Symmetry of multiplication-preserving order isomorphisms -/
@[to_additive (attr := symm) "Symmetry of addition-preserving order isomorphisms"]
def symm (f : α ≃*o β) : β ≃*o α where
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Surprising that we didn't have this before but it seems to indeed have been the case!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This ended up being added independently in #22402


/-- A `≤`-preserving group homomorphism `Γ₀ → Γ'₀` induces a map `Valuation R Γ₀ → Valuation R Γ'₀`.
-/
def map (f : Γ₀ →*₀ Γ'₀) (hf : Monotone f) (v : Valuation R Γ₀) : Valuation R Γ'₀ :=
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are these changes supposed to be included?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No indeed, I will fix when I get back to that PR (possibly in a while)

@YaelDillies
Copy link
Copy Markdown
Contributor Author

I am not touching this PR for a while by lack of time. Feel free to take over if you feel like it!

@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 Feb 11, 2025
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 12, 2025
@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 Mar 11, 2025
@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 Mar 12, 2025
@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 Apr 11, 2025
@leanprover-community-bot-assistant
Copy link
Copy Markdown
Collaborator

This pull request has conflicts, please merge master and resolve them.

@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Apr 11, 2025
@urkud
Copy link
Copy Markdown
Member

urkud commented Apr 11, 2025

Please add some explanations to the commit message.

YaelDillies added a commit that referenced this pull request May 11, 2025
It's a big issue that this coercion even exists, but it is difficult to remove at the moment, cf #21031.
mathlib-bors bot pushed a commit that referenced this pull request May 11, 2025
It's a big issue that this coercion even exists, but it is difficult to remove at the moment, cf #21031.

From Toric
tannerduve pushed a commit that referenced this pull request May 13, 2025
It's a big issue that this coercion even exists, but it is difficult to remove at the moment, cf #21031.

From Toric
bwehlin pushed a commit to bwehlin/mathlib4 that referenced this pull request May 31, 2025
…rover-community#24779)

It's a big issue that this coercion even exists, but it is difficult to remove at the moment, cf leanprover-community#21031.

From Toric
@mathlib4-merge-conflict-bot
Copy link
Copy Markdown
Collaborator

This pull request has conflicts, please merge master and resolve them.

@mathlib4-merge-conflict-bot mathlib4-merge-conflict-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 11, 2025
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 12, 2025
@mathlib4-merge-conflict-bot
Copy link
Copy Markdown
Collaborator

This pull request has conflicts, please merge master and resolve them.

@mathlib4-merge-conflict-bot mathlib4-merge-conflict-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 13, 2025
@mathlib4-merge-conflict-bot mathlib4-merge-conflict-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 13, 2025
@Vierkantor Vierkantor self-assigned this Oct 23, 2025
@mathlib4-merge-conflict-bot mathlib4-merge-conflict-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Nov 19, 2025
@mathlib4-merge-conflict-bot
Copy link
Copy Markdown
Collaborator

This pull request has conflicts, please merge master and resolve them.

@mathlib4-merge-conflict-bot mathlib4-merge-conflict-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Nov 19, 2025
@Vierkantor
Copy link
Copy Markdown
Contributor

What is the status of this PR? Because this is work that we want to get into Mathlib at some point, but I've been holding off on reviewing because of the empty PR description and WIP tag.

@YaelDillies
Copy link
Copy Markdown
Contributor Author

The status is that it needs a lot more work before it's reviewable. I'm expecting the final diff to be around +-3k lines.

@j-loreaux suggested preliminary steps to this PR but I am not sure that one can actually take those steps before the whole PR.

Concretely, he suggests to first remove uses of hom classes in definitions, and then remove the generic coercions. The issue I see is that this will change the syntactic form of a lot of lemmas, and simp won't yet have been upgraded to handle that. My suggestion instead is to do the refactor one type of morphisms at a time, starting from the leaves of the morphism hierarchy and working towards the roots.

@mathlib4-merge-conflict-bot mathlib4-merge-conflict-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 1, 2026
@mathlib4-merge-conflict-bot
Copy link
Copy Markdown
Collaborator

This pull request has conflicts, please merge master and resolve them.

@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 1, 2026
@mathlib-merge-conflicts mathlib-merge-conflicts bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 4, 2026
@mathlib-merge-conflicts
Copy link
Copy Markdown

This pull request has conflicts, please merge master and resolve them.

@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants