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] - refactor(algebra/group/defs): remove left-right_cancel_comm_monoids #7134

Closed
wants to merge 17 commits into from

Conversation

adomani
Copy link
Collaborator

@adomani adomani commented Apr 9, 2021

There were 6 distinct classes

  • (add_)left_cancel_comm_monoid,
  • (add_)right_cancel_comm_monoid,
  • (add_)cancel_comm_monoid.

I removed them all, except for the last 2:

  • (add_)cancel_comm_monoid.

The new typeclass cancel_comm_monoid requires only a * b = a * c → b = c, and deduces the other version from commutativity.


Please, can you really make sure that I have not messed up? Thanks! In particular, I have moved the attribute [to_additive] after the various typeclasses in question (UPD: Yury moved them back). For more information, see the Zulip discussion.

Many thanks to Anne, Eric, Kevin and Mario for their explanations! I have made an effort to understand them, but large parts of them are still mysterious to me!

Zulip discussion:
https://leanprover.zulipchat.com/#narrow/stream/113489-new-members/topic/mul_right_comm_monoid

Co-authored-by: Yury G. Kudryashov urkud@urkud.name

Open in Gitpod

@adomani adomani added the awaiting-review The author would like community review of the PR label Apr 9, 2021
src/algebra/group/defs.lean Outdated Show resolved Hide resolved
src/algebra/group/defs.lean Outdated Show resolved Hide resolved
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@adomani
Copy link
Collaborator Author

adomani commented Apr 10, 2021

Thanks, Anne: I pushed your suggestions!

@urkud
Copy link
Member

urkud commented Apr 12, 2021

I'll push a few fixes soon (UPD: done). The most important: cancel_comm_monoid should only extend left_cancel_monoid, not cancel_monoid. There should be an instance to_cancel_monoid that proves mul_right_cancel from commutativity.

@urkud
Copy link
Member

urkud commented Apr 12, 2021

bors d+

@adomani
Copy link
Collaborator Author

adomani commented Apr 12, 2021

Yury, thank you very much for the fixes!

Is the guiding principle that you want to have as few fields as possible? So no need to assume right cancel, given left and comm?

@bors
Copy link

bors bot commented Apr 12, 2021

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

@urkud urkud changed the title unfeat(algebra/group/defs): remove left-right_cancel_comm_monoids refactor(algebra/group/defs): remove left-right_cancel_comm_monoids Apr 12, 2021
remove `mul_right_cancel` from ordered_cancel_comm_monoid
remove `mul_right_cancel` from linear_ordered_comm_group.to_linear_ordered_cancel_comm_monoid
remove `mul_right_cancel` from ordered_comm_group.to_ordered_cancel_comm_monoid
remove several add_right_cancel
@urkud
Copy link
Member

urkud commented Apr 12, 2021

Yury, thank you very much for the fixes!

Is the guiding principle that you want to have as few fields as possible? So no need to assume right cancel, given left and comm?

Yes, users who define instances should have to provide as few fields as possible. We don't follow this rule everywhere (e.g., the axioms for group are not minimal) but we do it if it's easy.

@adomani
Copy link
Collaborator Author

adomani commented Apr 12, 2021

Wow, Yury: at this rate, mathlib will be less than half its size in a matter of hours!

Thank you so much for the changes: I thought that they were going to be all simple removal of unneeded fields, but it would have taken me quite a while to fix some of these errors!

Thanks for all your help!

I imagine that, as soon as the PR builds successfully it can be merged, right?

@adomani
Copy link
Collaborator Author

adomani commented Apr 12, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 12, 2021
…7134)

There were 6 distinct classes
* `(add_)left_cancel_comm_monoid`,
* `(add_)right_cancel_comm_monoid`,
* `(add_)cancel_comm_monoid`.

I removed them all, except for the last 2:
* `(add_)cancel_comm_monoid`.

The new typeclass `cancel_comm_monoid` requires only `a * b = a * c → b = c`, and deduces the other version from commutativity.



Co-authored-by: Yury G. Kudryashov <urkud@urkud.name>
@bors
Copy link

bors bot commented Apr 13, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(algebra/group/defs): remove left-right_cancel_comm_monoids [Merged by Bors] - refactor(algebra/group/defs): remove left-right_cancel_comm_monoids Apr 13, 2021
@bors bors bot closed this Apr 13, 2021
@bors bors bot deleted the adomani_left-right_cancel_comm branch April 13, 2021 03:56
@adomani adomani removed the awaiting-review The author would like community review of the PR label Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants