Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

[Merged by Bors] - refactor(algebra/gcd_monoid): don't require normalization#9443

Closed
Ruben-VandeVelde wants to merge 27 commits intomasterfrom
gcd-no-norm
Closed

[Merged by Bors] - refactor(algebra/gcd_monoid): don't require normalization#9443
Ruben-VandeVelde wants to merge 27 commits intomasterfrom
gcd-no-norm

Conversation

@Ruben-VandeVelde
Copy link
Collaborator

@Ruben-VandeVelde Ruben-VandeVelde commented Sep 29, 2021

This will allow us to set up a gcd_monoid instance for all euclidean_domains and generalize the results in ring_theory/euclidean_domain.lean to PIDs.


This still needs some documentation and lint fixes, and I didn't pay much attention to names yet, but I'd like to hear if people think this is the right direction first.

Open in Gitpod

This will allow us to set up a gcd_monoid instance for all euclidean_domains and generalize the results in ring_theory/euclidean_domain.lean to PIDs.
@Ruben-VandeVelde Ruben-VandeVelde added the awaiting-review The author would like community review of the PR label Sep 29, 2021
@@ -208,44 +211,53 @@ end associates
supremum, `1` is bottom, and `0` is top. The type class focuses on `gcd` and we derive the
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't a bounded lattice require lcm and gcd be commutative and associative, which is no longer true of the weaker typeclass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seems plausible - I can't find an actual Lean instance, though, so I'm not sure what this comment is referring to.

Copy link
Member

Choose a reason for hiding this comment

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

Do the generalisations you care about not have associativity or commutativity either?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No commutativity, in any case:

import algebra.euclidean_domain

#eval euclidean_domain.gcd (1 : int) (-1 : int)
-- 1
#eval euclidean_domain.gcd (-1 : int) (1 : int)
-- -1

@kim-em
Copy link
Collaborator

kim-em commented Oct 1, 2021

Please don't write /-- TODO -/. :-) The whole point of the linter is to warn you that these are still TODO.

I'll mark as WIP for now, for you to add the doc-strings, but otherwise this is looking good!

@kim-em kim-em added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Oct 1, 2021
@jcommelin jcommelin added the needs-documentation This PR is missing required documentation label Oct 1, 2021
@Ruben-VandeVelde Ruben-VandeVelde added awaiting-review The author would like community review of the PR and removed WIP Work in progress needs-documentation This PR is missing required documentation labels Oct 1, 2021
@Ruben-VandeVelde Ruben-VandeVelde 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 Oct 1, 2021
Copy link
Collaborator

@awainverse awainverse left a comment

Choose a reason for hiding this comment

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

I left some small recommendations. This seems like a good idea overall.
In particular, this doesn't get rid of the existing functionality of the existing gcd_monoid, just renames it, so I'm happy.

I think it would also be nice to tie a gcd_monoid structure on M to a normalized_gcd_monoid structure on associates M, but that should probably wait for a later PR because this one is a lot of lines!

@Ruben-VandeVelde
Copy link
Collaborator Author

I think it would also be nice to tie a gcd_monoid structure on M to a normalized_gcd_monoid structure on associates M, but that should probably wait for a later PR because this one is a lot of lines!

Yeah, I'm happy to do that after this.

@kim-em kim-em added awaiting-author A reviewer has asked the author a question or requested changes awaiting-CI The author would like to see what CI has to say before doing more work. and removed awaiting-review The author would like community review of the PR labels Oct 4, 2021
@Ruben-VandeVelde Ruben-VandeVelde 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 awaiting-CI The author would like to see what CI has to say before doing more work. labels Oct 4, 2021
@kim-em
Copy link
Collaborator

kim-em commented Oct 5, 2021

bors d+

@bors
Copy link

bors bot commented Oct 5, 2021

✌️ Ruben-VandeVelde 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 The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Oct 5, 2021
@Ruben-VandeVelde
Copy link
Collaborator Author

bors merge

Thanks!

bors bot pushed a commit that referenced this pull request Oct 5, 2021
This will allow us to set up a gcd_monoid instance for all euclidean_domains and generalize the results in ring_theory/euclidean_domain.lean to PIDs.
@bors
Copy link

bors bot commented Oct 5, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(algebra/gcd_monoid): don't require normalization [Merged by Bors] - refactor(algebra/gcd_monoid): don't require normalization Oct 5, 2021
@bors bors bot closed this Oct 5, 2021
@bors bors bot deleted the gcd-no-norm branch October 5, 2021 11:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

delegated The PR author may merge after reviewing final suggestions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants