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] - feat(GroupTheory/GroupAction): define MulAction.period and create GroupTheory/GroupAction/Period #9490

Closed
wants to merge 13 commits into from

Conversation

adri326
Copy link
Collaborator

@adri326 adri326 commented Jan 6, 2024

Defines MulAction.period g a as a wrapper around Function.minimalPeriod (fun x => g • x) a,
allowing for some cleaner proofs involving the period of a group action.
The existing MulAction.*_minimalPeriod_* lemmas are changed to be defined using their now-preferred MulAction.*_period_* counterparts,
although they will only be made deprecated in another pull request.
The Mathlib.GroupTheory.GroupAction.Period module is also created, for additional lemmas around MulAction.period.

Some core lemmas need to remain in Mathlib.Dynamics.PeriodicPts, as they are needed for ZMod and the quotient group.


Note that the linter fails on Mathlib.Dynamic.PeriodicPts, since I do not know how to split the to_additive description on multiples lines :/

Open in Gitpod

@adri326 adri326 added awaiting-review The author would like community review of the PR t-algebra Algebra (groups, rings, fields etc) t-dynamics Dynamical Systems new-contributor This PR was made by a contributor with fewer than 5 merged PRs. Welcome to the community! labels Jan 6, 2024
@erdOne
Copy link
Member

erdOne commented Jan 6, 2024

Note that the linter fails on Mathlib.Dynamic.PeriodicPts, since I do not know how to split the to_additive description on multiples lines :/

I think a newline without indentation works, i.e.

@[to_additive "foo
bar"]
def 

@adri326
Copy link
Collaborator Author

adri326 commented Jan 6, 2024

Oh it does, thanks

Copy link
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.

Some stylistic comments

Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
@adri326
Copy link
Collaborator Author

adri326 commented Jan 7, 2024

Fixed the style issues pointed out above :)

@Vierkantor Vierkantor self-assigned this Feb 13, 2024
Copy link
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.

Thanks for the PR. It might look like I have a lot of comments, but I'm confident that they'll be the (almost) complete set, once they're done everything is ready to merge. This PR is nicely structured overall!

Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/Period.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/Period.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/Period.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/Period.lean Outdated Show resolved Hide resolved
Mathlib/Dynamics/PeriodicPts.lean Show resolved Hide resolved
@Vierkantor Vierkantor 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 Feb 13, 2024
@adri326
Copy link
Collaborator Author

adri326 commented Feb 14, 2024

Couple of changes, as suggested; of interest:

  • le_period_of_movedle_period
  • moved_of_lt_periodpow_smul_ne_of_lt_period (currently lives in the MulAction namespace)
  • period_eq_one_of_fixedperiod_eq_one_iff
  • new period_one and period_inv (I couldn't golf the latter any further)
  • new period_dvd_orderOf and period_dvd_exponent, which helped golfed the rest of the lemmas
  • added missing docstrings in @[to_additive]

@adri326 adri326 added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Feb 14, 2024
Copy link
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.

Some minor comments and I'm happy with this, thanks! I'm going to ask another maintainer to delegate this because I haven't reviewed Dynamics before and I contributed a few of the lemmas.

maintainer merge

Mathlib/Dynamics/PeriodicPts.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/Period.lean Outdated Show resolved Hide resolved
Copy link

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

1 similar comment
Copy link

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

adri326 and others added 2 commits February 15, 2024 12:47
…ve and move angle bracket

Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

bors merge

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Feb 15, 2024
mathlib-bors bot pushed a commit that referenced this pull request Feb 15, 2024
…upTheory/GroupAction/Period (#9490)

Defines `MulAction.period g a` as a wrapper around `Function.minimalPeriod (fun x => g • x) a`,
allowing for some cleaner proofs involving the period of a group action.
The existing `MulAction.*_minimalPeriod_*` lemmas are changed to be defined using their now-preferred `MulAction.*_period_*` counterparts,
although they will only be made deprecated in another pull request.
The `Mathlib.GroupTheory.GroupAction.Period` module is also created, for additional lemmas around `MulAction.period`.

Some core lemmas need to remain in `Mathlib.Dynamics.PeriodicPts`, as they are needed for `ZMod` and the quotient group.
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 15, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(GroupTheory/GroupAction): define MulAction.period and create GroupTheory/GroupAction/Period [Merged by Bors] - feat(GroupTheory/GroupAction): define MulAction.period and create GroupTheory/GroupAction/Period Feb 15, 2024
@mathlib-bors mathlib-bors bot closed this Feb 15, 2024
@mathlib-bors mathlib-bors bot deleted the adri326/mulaction-period branch February 15, 2024 14:03
riccardobrasca pushed a commit that referenced this pull request Feb 18, 2024
…upTheory/GroupAction/Period (#9490)

Defines `MulAction.period g a` as a wrapper around `Function.minimalPeriod (fun x => g • x) a`,
allowing for some cleaner proofs involving the period of a group action.
The existing `MulAction.*_minimalPeriod_*` lemmas are changed to be defined using their now-preferred `MulAction.*_period_*` counterparts,
although they will only be made deprecated in another pull request.
The `Mathlib.GroupTheory.GroupAction.Period` module is also created, for additional lemmas around `MulAction.period`.

Some core lemmas need to remain in `Mathlib.Dynamics.PeriodicPts`, as they are needed for `ZMod` and the quotient group.
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
…upTheory/GroupAction/Period (#9490)

Defines `MulAction.period g a` as a wrapper around `Function.minimalPeriod (fun x => g • x) a`,
allowing for some cleaner proofs involving the period of a group action.
The existing `MulAction.*_minimalPeriod_*` lemmas are changed to be defined using their now-preferred `MulAction.*_period_*` counterparts,
although they will only be made deprecated in another pull request.
The `Mathlib.GroupTheory.GroupAction.Period` module is also created, for additional lemmas around `MulAction.period`.

Some core lemmas need to remain in `Mathlib.Dynamics.PeriodicPts`, as they are needed for `ZMod` and the quotient group.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-contributor This PR was made by a contributor with fewer than 5 merged PRs. Welcome to the community! ready-to-merge This PR has been sent to bors. t-algebra Algebra (groups, rings, fields etc) t-dynamics Dynamical Systems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants