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): new FixedPoints module with a few basic properties of fixedBy #9477

Closed
wants to merge 17 commits into from

Conversation

adri326
Copy link
Collaborator

@adri326 adri326 commented Jan 6, 2024

Introduces a new module, Mathlib.GroupTheory.GroupAction.FixedPoints,
which contains some properties of MulAction.fixedBy that wouldn't quite belong to Mathlib.GroupTheory.GroupAction.Basic.


This is my first contribution ever to Mathlib, and the first contribution in a series of contributions to eventually merge my formalization of Rubin's theorem into Lean.

I have a few questions around the design decisions that I took:

  • should movedBy be defined in Mathlib.GroupTheory.GroupAction.Basic instead?
  • Mathlib.GroupTheory.GroupAction.FixingSubgroup uses M for a group, would it be out-of-scope for this PR to also replace it with G?
  • should I try to further minimize the usage of tactic groups?
  • I'm trying to translate the properties that I will need later for the formalization of Rubin's theorem, so I wonder if not_commute_of_disjoint_movedBy_preimage might be too out-of-scope

Open in Gitpod

…group is a subset

This is a theorem that is needed for the proof of Rubin's theorem,
and it sets the ground up for discussions about implementing a separate `movingSubgroup`
definition.
…gation and pointwise SMul

Namely:
- `fixedBy_mul`, `movedBy_mul`
- `fixedBy_conj`, `movedBy_conj`
- `smul_pow_mem_of_movedBy_subset` and its corollaries
- `smul_pow_mem_movedBy_of_commute` and its corollaries
@adri326 adri326 added t-algebra Algebra (groups, rings, fields etc) new-contributor This PR was made by a contributor with fewer than 5 merged PRs. Welcome to the community! labels Jan 6, 2024
@adri326 adri326 added the awaiting-review The author would like community review of the PR label Jan 6, 2024
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
@jcommelin jcommelin 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 Jan 6, 2024
@adri326
Copy link
Collaborator Author

adri326 commented Jan 6, 2024

I fixed the above issues; the CI should pass, as I haven't changed any public-facing interfaces.

@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 Jan 6, 2024
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
@adri326
Copy link
Collaborator Author

adri326 commented Jan 14, 2024

I fixed the issues pointed out by @riccardobrasca, as well as some I noticed in the meantime (some theorems were named pow when they referred to zpow).

I would like to say that trying to get this first pull request merged has been a pretty demotivating experience so far:
there is no proper linter or formatter that I could run locally,
building documentation locally is extremely slow and the instructions for doing so are poorly documented, etc.

The backlog of pull requests means that each round of small issue that could be caught ahead of time with proper tooling instead adds at least a day before this pull request gets approved. I understand the need to have PRs be properly reviewed, but I long for tools to reduce these reviews down to the more impactful issues.

@riccardobrasca
Copy link
Member

I fixed the issues pointed out by @riccardobrasca, as well as some I noticed in the meantime (some theorems were named pow when they referred to zpow).

I would like to say that trying to get this first pull request merged has been a pretty demotivating experience so far: there is no proper linter or formatter that I could run locally, building documentation locally is extremely slow and the instructions for doing so are poorly documented, etc.

The backlog of pull requests means that each round of small issue that could be caught ahead of time with proper tooling instead adds at least a day before this pull request gets approved. I understand the need to have PRs be properly reviewed, but I long for tools to reduce these reviews down to the more impactful issues.

I am sorry this has been a painful experience.

One thing to keep in mind is that last week there was a pretty important Lean conference, so a lot of people didn't have time to review PRs: we know that it is a slow process, but maybe you were just unlucky.

Can you please copy/paste this message on Zulip? It is very possible that some of tools you want exist, but are maybe not very well documented (for example you can run all the linters locally, it is just slow).

Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
@semorrison
Copy link
Contributor

(for example you can run all the linters locally, it is just slow).

If you have enough local compute make lint is all that it takes to run the linter locally.

Automatic code formatting is definitely on the roadmap!

…complement of fixedBy

This is in an effort to keep definitions simp-normal: `movedBy α g` meant that
simp lemmas need to be duplicated between `fixedBy` and `movedBy`.
@adri326
Copy link
Collaborator Author

adri326 commented Jan 18, 2024

I replaced movedBy α g with (fixedBy α g)ᶜ, and added some helper lemmas around s ∈ fixedBy (Set α) g. I tried having abbrev but it made rw only work sporadically, and I couldn't get notation to work like I wanted to.

This was quite the refactor so I wouldn't be surprised if I accidentally re-introduced small issues in there.

@adri326 adri326 changed the title feat(GroupTheory/GroupAction): MulAction.movedBy and a few basic properties of fixedBy and movedBy feat(GroupTheory/GroupAction): new FixedPoints module with a few basic properties of fixedBy Feb 5, 2024
@adri326
Copy link
Collaborator Author

adri326 commented Feb 5, 2024

@tb65536 haven't heard back in a little while, are there other design patterns that are problematic with this PR?

Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixedPoints.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixingSubgroup.lean Outdated Show resolved Hide resolved
Copy link
Collaborator

@tb65536 tb65536 left a comment

Choose a reason for hiding this comment

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

Last really minor comments, but this is looking good to me

Mathlib/GroupTheory/GroupAction/FixingSubgroup.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixingSubgroup.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixingSubgroup.lean Outdated Show resolved Hide resolved
Mathlib/GroupTheory/GroupAction/FixingSubgroup.lean Outdated Show resolved Hide resolved
@tb65536
Copy link
Collaborator

tb65536 commented Feb 7, 2024

LGTM! Thanks for adding all this API

maintainer merge

Copy link

github-actions bot commented Feb 7, 2024

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

@tb65536
Copy link
Collaborator

tb65536 commented Feb 7, 2024

(I guess it should be delegated, since CI is still running)

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!

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 12, 2024
mathlib-bors bot pushed a commit that referenced this pull request Feb 12, 2024
…sic properties of `fixedBy` (#9477)

Introduces a new module, `Mathlib.GroupTheory.GroupAction.FixedPoints`,
which contains some properties of `MulAction.fixedBy` that wouldn't quite belong to `Mathlib.GroupTheory.GroupAction.Basic`.
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 12, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(GroupTheory/GroupAction): new FixedPoints module with a few basic properties of fixedBy [Merged by Bors] - feat(GroupTheory/GroupAction): new FixedPoints module with a few basic properties of fixedBy Feb 12, 2024
@mathlib-bors mathlib-bors bot closed this Feb 12, 2024
@mathlib-bors mathlib-bors bot deleted the adri326/mulaction-movedby branch February 12, 2024 11:39
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
…sic properties of `fixedBy` (#9477)

Introduces a new module, `Mathlib.GroupTheory.GroupAction.FixedPoints`,
which contains some properties of `MulAction.fixedBy` that wouldn't quite belong to `Mathlib.GroupTheory.GroupAction.Basic`.
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants