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

refactor(algebra/group/type_tags): make additive and multiplicative structures #6045

Closed
wants to merge 41 commits into from

Conversation

gebner
Copy link
Member

@gebner gebner commented Feb 4, 2021


This is an alternative to #2292, going one step further and making the two structures instead of irreducible. I'd mainly like feedback before sinking more time into this.

I would have expected this refactoring to be more horrible. But I find that instead I can actually understand the new proofs, since they don't depend on the fact that 0 reduces to 1 or vice versa, but instead on equational lemmas that I can look at. Also equiv_rw is really helpful here.

Zulip discussion

@gebner gebner added awaiting-review The author would like community review of the PR mathport For compatibility with Lean 4 changes, to simplify porting labels Feb 4, 2021
@gebner gebner marked this pull request as draft February 4, 2021 17:38
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Feb 13, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 4, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 7, 2021
@bryangingechen
Copy link
Collaborator

@gebner do you mind expanding the module doc in algebra/group/type_tags to explain how additive and multiplicative should be used now? Examples would be appreciated too. (The module doc didn't say much on this before, but I figure now is as good a time as any to request this.)

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 8, 2021
@eric-wieser
Copy link
Member

This will need a careful merge with #6572, as that replaced section-level typeclasses with lemma-level ones.

@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 8, 2021
@gebner
Copy link
Member Author

gebner commented Mar 8, 2021

This will need a careful merge with #6572, as that replaced section-level typeclasses with lemma-level ones.

I hope I didn't revert anything.

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.

The merge of that PR looks clean, thanks!

Copy link
Collaborator

@bryangingechen bryangingechen left a comment

Choose a reason for hiding this comment

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

The new documentation looks good to me, and the new proofs do look easier to understand as well. This is a big refactor though so I wouldn't mind getting more opinions.

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 11, 2021
@github-actions github-actions bot added merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Mar 11, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 12, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 16, 2021
@fpvandoorn fpvandoorn 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 Aug 20, 2021
@eric-wieser
Copy link
Member

xref leanprover/lean4#777 for some discussion about the cost of making these structures.

@YaelDillies
Copy link
Collaborator

The discussion now pertains to mathlib4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes mathport For compatibility with Lean 4 changes, to simplify porting merge-conflict Please `git merge origin/master` then a bot will remove this label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants