Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

refactor(group_theory/perm/basic): Redefine pow in terms of iterate #18077

Closed
wants to merge 5 commits into from

Conversation

YaelDillies
Copy link
Collaborator

@YaelDillies YaelDillies commented Jan 6, 2023

Before, equiv.perm inherited the default pow. This changes it to a custom one using function.iterate.

Unfortunately, this changes the defeq as function.iterate is left recursive while npow_rec is right recursive. See Zulip: https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/.60nat.2Eiterate.60.20vs.20.60stream.2Eiterate.60


Match leanprover-community/mathlib4#1388

Open in Gitpod

@YaelDillies YaelDillies added awaiting-CI The author would like to see what CI has to say before doing more work. awaiting-review The author would like community review of the PR t-algebra Algebra (groups, rings, fields etc) labels Jan 6, 2023
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Can you do the same thing for function.End? module.End etc would be good too, but as those aren't "simpler" than perm they can wait to a follow-up.

@eric-wieser
Copy link
Member

Did you consider changing the defeq of function.iterate or npow_rec? If such a change is seamless, it might be the easier solution...

@YaelDillies
Copy link
Collaborator Author

I remember lengthy discussions about why nat.iterate is tail-recursive. The answer is basically computation, so we're not really allowed to change it. Changing the defeq for npow_rec wouldn't help much. It would only have helped if it had been different in the first place.

@YaelDillies
Copy link
Collaborator Author

Done function.End. monoid_hom.End looks harder to get to because of import cycles (I would need algebra.hom.iterate, which imports group_theory.group_action.opposite which imports group_theory.group_action.defs).

@eric-wieser
Copy link
Member

I remember lengthy discussions about why nat.iterate is tail-recursive.

That would be great to have in the commit message if yo ucan find it

Changing the defeq for npow_rec wouldn't help much.

Why not? Does too much stuff depend on the current definition?

@YaelDillies
Copy link
Collaborator Author

YaelDillies commented Jan 7, 2023

Yes exactly. Many proofs rely on the current defeq.

Here is the discussion: https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/.60nat.2Eiterate.60.20vs.20.60stream.2Eiterate.60

@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jan 7, 2023
@YaelDillies
Copy link
Collaborator Author

Well, I guess not that many in the end 😄

@eric-wieser
Copy link
Member

Changing the defeq for npow_rec wouldn't help much.

I realized on Zulip that it wouldn't help at all; this relies on function application commuting with recursors definitionally, which it does not.

@eric-wieser
Copy link
Member

Done function.End. monoid_hom.End looks harder to get to because of import cycles (I would need algebra.hom.iterate, which imports group_theory.group_action.opposite which imports group_theory.group_action.defs).

Can we compromise on adding a TODO comment at the top of algebra.hom.iterate?

@urkud
Copy link
Member

urkud commented Feb 16, 2023

Note that if we go forward with this refactor, the monoid structure on category_theory.End (Group.of G) will be no longer defeq to monoid.End.monoid. I don't know whether we ever rely on this defeq.

@kim-em
Copy link
Collaborator

kim-em commented Mar 26, 2023

Could we abandon this as a mathlib3 PR, and make any desired changes after the port?

@YaelDillies
Copy link
Collaborator Author

I don't know. This is pretty short, self-contained and ready.

@eric-wieser
Copy link
Member

Note that if we go forward with this refactor, the monoid structure on category_theory.End (Group.of G) will be no longer defeq to monoid.End.monoid. I don't know whether we ever rely on this defeq.

Is this fixable?

@YaelDillies
Copy link
Collaborator Author

@kim-em kim-em added the modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. label Mar 28, 2023
@urkud
Copy link
Member

urkud commented Jun 22, 2023

No, it isn't. category_theory deals with non-necessarily concrete categories. Even if we're talking about concrete categories, we can't insert nat.iterate inside an unknown bundled homomorphism. The only way to fix it I can see is to add npow data to the category_theory.category signature. I don't think that we want to do this. IMHO, it's better to deal with a non-definitional equality in a visible argument than to have it in typeclass instances.

@urkud urkud added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Jun 22, 2023
@kim-em kim-em added the too-late This PR was ready too late for inclusion in mathlib3 label Jul 16, 2023
@YaelDillies YaelDillies deleted the perm_pow_iterate branch April 20, 2024 13:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting-author A reviewer has asked the author a question or requested changes modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. t-algebra Algebra (groups, rings, fields etc) too-late This PR was ready too late for inclusion in mathlib3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants