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: port GroupTheory/Submonoid/Basic #1224

Closed
wants to merge 9 commits into from

Conversation

ADedecker
Copy link
Member

@ADedecker ADedecker commented Dec 26, 2022

No description provided.

@ADedecker ADedecker added WIP Work in progress mathlib-port This is a port of a theory file from mathlib. labels Dec 26, 2022
@ADedecker ADedecker added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Dec 26, 2022
@ChrisHughes24 ChrisHughes24 added delegated and removed awaiting-review The author would like community review of the PR labels Dec 26, 2022
@ChrisHughes24
Copy link
Member

bors d+

@bors
Copy link

bors bot commented Dec 26, 2022

✌️ ADedecker can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
@ChrisHughes24 ChrisHughes24 added awaiting-author A reviewer has asked the author a question or requested changes and removed delegated labels Dec 27, 2022
@urkud
Copy link
Member

urkud commented Dec 27, 2022

  • The loop in pow_mem can be resolved if you add : Prop to MulMemClass etc.
  • eq_mlocus etc were there for submonoids so that we can use non-m versions for subgroups.

Please ping me on Zulip when this is ready for review again. I want to have another look before it's merged.

@urkud
Copy link
Member

urkud commented Dec 27, 2022

bors d-

@ADedecker
Copy link
Member Author

The loop in pow_mem can be resolved if you add : Prop to MulMemClass etc.

That didn't make the situation better on my side 🤔
What did however was changing some arguments from [] to {} in SubmonoidClass.toOneMemClass

eq_mlocus etc were there for submonoids so that we can use non-m versions for subgroups.

Yes I know that, but I saw you removed some of them when porting GroupTheory.Subsemigroup.Basic. Is it because they weren't supposed to be there in the first place? Anyway, I'll add them back

@ADedecker ADedecker added awaiting-review The author would like community review of the PR delegated and removed delegated awaiting-author A reviewer has asked the author a question or requested changes labels Dec 28, 2022
@urkud
Copy link
Member

urkud commented Dec 28, 2022

eq_mlocus etc were there for submonoids so that we can use non-m versions for subgroups.

Yes I know that, but I saw you removed some of them when porting GroupTheory.Subsemigroup.Basic. Is it because they weren't supposed to be there in the first place? Anyway, I'll add them back

I think that somebody copied them from submonoid to subsemigroup in mathlib. IMHO, they shouldn't be there.

@urkud
Copy link
Member

urkud commented Dec 28, 2022

Thanks! I think that these typeclasses should be moved to Prop but this can wait for leanprover-community/mathlib#18015 to be merged.
bors d+

@bors
Copy link

bors bot commented Dec 28, 2022

✌️ ADedecker can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot added delegated and removed awaiting-review The author would like community review of the PR labels Dec 28, 2022
@ADedecker
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Dec 28, 2022
Co-authored-by: Yury G. Kudryashov <urkud@urkud.name>
@bors
Copy link

bors bot commented Dec 28, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: port GroupTheory/Submonoid/Basic [Merged by Bors] - feat: port GroupTheory/Submonoid/Basic Dec 28, 2022
@bors bors bot closed this Dec 28, 2022
@bors bors bot deleted the AD_PORT_Submonoid_Basic branch December 28, 2022 18:30
@semorrison
Copy link
Contributor

@ADedecker, @urkud, @ChrisHughes24, just a heads up that this PR has lots of missing #aligns for the to_additive versions of statements. We should be checking for this when reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated mathlib-port This is a port of a theory file from mathlib.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants