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/ordered_group): another step in the order refactor -- ordered groups #8060

Closed
wants to merge 94 commits into from

Conversation

adomani
Copy link
Collaborator

@adomani adomani commented Jun 23, 2021

This PR represents another wave of generalization of proofs, following from the order refactor. It is another step towards #7645.


Open in Gitpod

@adomani adomani added the awaiting-review The author would like community review of the PR label Jun 29, 2021
Copy link
Member

@fpvandoorn fpvandoorn left a comment

Choose a reason for hiding this comment

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

In the first part you have lemmas which each of the four classes below. It would be good if you have the same pattern of lemmas for the four classes, following the same naming scheme (if possible while avoiding naming clashes):
[covariant_class α α (*) (≤)]
[covariant_class α α (*) (<)]
[covariant_class α α (function.swap (*)) (≤)]
[covariant_class α α (function.swap (*)) (<)]
There are currently quite some inconsistencies/omissions.

In some comments I ask about inconsistencies. To what extend is this because of trying to be (somewhat) backwards compatible?

src/algebra/ordered_group.lean Outdated Show resolved Hide resolved
src/algebra/ordered_group.lean Outdated Show resolved Hide resolved
src/algebra/ordered_group.lean Outdated Show resolved Hide resolved
src/algebra/ordered_group.lean Outdated Show resolved Hide resolved
src/algebra/ordered_group.lean Show resolved Hide resolved
src/algebra/ordered_group.lean Show resolved Hide resolved
by { rw [mul_inv_cancel_right, mul_comm a, mul_inv_cancel_right] at this, rw [this] }
by { rw [← mul_le_mul_iff_left a, ← mul_le_mul_iff_right b], simp }

alias neg_le_neg_iff ↔ le_of_neg_le_neg _
Copy link
Member

Choose a reason for hiding this comment

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

Here and in other places, there are aliases only for additive operations. Is this just because the library doesn't use the multiplicative versions of these lemmas, but it does use the additive version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, most of the times (probably all, I do not remember), I reformulated the statement from the additive version to multiplicative with to_additive, and then I only introduced the alias for the additive version, since the multiplicative version was not in Lean before anyway.

src/algebra/ordered_group.lean Outdated Show resolved Hide resolved
src/algebra/ordered_group.lean Outdated Show resolved Hide resolved
lemma le_abs_self (a : α) : a ≤ abs a := le_max_left _ _

lemma neg_le_abs_self (a : α) : -a ≤ abs a := le_max_right _ _

lemma neg_abs_le_self (a : α) : -abs a ≤ a :=
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that the shorter proof doesn't work anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very superficially, neg_le uses both left and right monotonicity. Here we are only assuming one-sided monotonicity and trading the other side for linear_order. This should explain the le_total bit. The calc proof makes it look longer than it really is, but that is another matter! Does this clarify why the old proof does not work?

@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 Jul 1, 2021
@adomani adomani 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 Jul 2, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Jul 6, 2021
b-mehta pushed a commit that referenced this pull request Jul 6, 2021
…a)variant` (#8072)

In an attempt to break off small parts of PR #8060, I extracted some of the instances proven there to this PR.

This is part of the `order` refactor.

~~I tried to get rid of the `@`, but failed.  If you have a trick to avoid them, I would be very happy to learn it!~~  `@`s removed!
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Jul 7, 2021
@fpvandoorn
Copy link
Member

Thanks for the changes!

bors d+

@bors
Copy link

bors bot commented Jul 7, 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.

@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 Jul 7, 2021
@fpvandoorn
Copy link
Member

bors merge

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jul 7, 2021
bors bot pushed a commit that referenced this pull request Jul 7, 2021
… -- ordered groups (#8060)

This PR represents another wave of generalization of proofs, following from the `order` refactor.  It is another step towards #7645.
@adomani
Copy link
Collaborator Author

adomani commented Jul 7, 2021

Floris, thank you so much for reviewing and for merging this! I think that the subsequent PRs will be shorter and more easily fractionable!

@bors
Copy link

bors bot commented Jul 7, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(algebra/ordered_group): another step in the order refactor -- ordered groups [Merged by Bors] - refactor(algebra/ordered_group): another step in the order refactor -- ordered groups Jul 7, 2021
@bors bors bot closed this Jul 7, 2021
@bors bors bot deleted the adomani_ordered_group branch July 7, 2021 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions. ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants