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] - docs(Algebra/Order/Ring/Defs): tiny IsDomain corrections #8489

Closed
wants to merge 3 commits into from

Conversation

madvorak
Copy link
Collaborator


Please check whether the new version is a correct description.

Open in Gitpod

@madvorak madvorak added the awaiting-review The author would like community review of the PR label Nov 18, 2023
@alexjbest
Copy link
Member

Actually it might be better to emphasize that IsDomain is a mixin now and not part of the heierachy. So it's not actually a predecessor in a literal sense

@madvorak
Copy link
Collaborator Author

Would you remove IsDomain from the comments, or change the wording?

@alexjbest
Copy link
Member

I would change the wording to Ring + IsDomain with linear order (or whatever the correct version is)

@madvorak madvorak 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 Nov 23, 2023
@madvorak
Copy link
Collaborator Author

madvorak commented Nov 24, 2023

I couldn't make sense out of it (i.e., I didn't find a combination IsDomain + something where IsDomain wouldn't be redundant for given purposes). If you find something, please, fill it in; otherwise, I better delete these two lines.

I am not saying it is ideal thing to do to the docs, but it is certainly better now than what there was before.

@madvorak madvorak 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 Nov 24, 2023
Copy link
Collaborator

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

It does make sense. I pushed a fix with the new appropriate description.

maintainer merge

Copy link

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

@riccardobrasca
Copy link
Member

Thanks!

bors d+

@mathlib-bors
Copy link

mathlib-bors bot commented Dec 12, 2023

✌️ madvorak 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 12, 2023
@madvorak
Copy link
Collaborator Author

Thank you @YaelDillies !

Shouldn't " & + respects & * respects < " be there as well?
I am not sure what exactly implies the rest of the properties.

@YaelDillies
Copy link
Collaborator

The point was to relate to non-order properties, so no it wouldn't make much sense.

@madvorak
Copy link
Collaborator Author

The point was to relate to non-order properties, so no it wouldn't make much sense.

Should it be removed from lines 68, 74, 81, 88 then?

@YaelDillies
Copy link
Collaborator

No, I mean the point of those two specific lines. Each item in a sublist removes one condition from the corresponding header. The ones about IsDomain are precisely removing the compatibility between algebraic and order operations.

@madvorak
Copy link
Collaborator Author

Sorry; it seems I don't understand what IsDomain does. If you are sure it is now correct, I will trust you and merge it as is.

@YaelDillies
Copy link
Collaborator

IsDomain just says that a * b = 0 → a = 0 ∨ b = 0. This is a consequence of linearity of the order + compatibility of the algebraic operations, but it is a purely algebraic condition.

@madvorak
Copy link
Collaborator Author

And what implies that + respects then?

@YaelDillies
Copy link
Collaborator

I think you are misunderstanding the point of this whole paragraph in the docs. It's supposed to answer questions of the form "I stated a lemma using LinearOrderedRing but never used the compatibility of * with for anything else than a * b = 0 → a = 0 ∨ b = 0. What should I do?", the answer to which would be "Use Ring + IsDomain + maybe LinearOrder".

@madvorak
Copy link
Collaborator Author

Yeah, I misunderstood it totally!

Should I merge it now as it is?

@YaelDillies
Copy link
Collaborator

The docstring did slightly go out of date (since Domain is no more) and I'm happy with this PR in its current form, so please do!

@madvorak
Copy link
Collaborator Author

bors r+

mathlib-bors bot pushed a commit that referenced this pull request Dec 12, 2023
Co-authored-by: Yaël Dillies <yael.dillies@gmail.com>
@mathlib-bors
Copy link

mathlib-bors bot commented Dec 12, 2023

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title docs(Algebra/Order/Ring/Defs): tiny IsDomain corrections [Merged by Bors] - docs(Algebra/Order/Ring/Defs): tiny IsDomain corrections Dec 12, 2023
@mathlib-bors mathlib-bors bot closed this Dec 12, 2023
@mathlib-bors mathlib-bors bot deleted the dd2moreletters branch December 12, 2023 13:55
awueth pushed a commit that referenced this pull request Dec 19, 2023
Co-authored-by: Yaël Dillies <yael.dillies@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants