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] - feat(algebra/order/monoid_lemmas_zero_lt): port some lemmas from algebra.order.monoid_lemmas #12961

Closed
wants to merge 4 commits into from

Conversation

negiizhao
Copy link
Collaborator

Although the names of these lemmas are from algebra.order.monoid_lemmas, many of the lemmas in algebra.order.monoid_lemmas have incorrect names, and some others may not be appropriate. Also, many lemmas are missing in algebra.order.monoid_lemmas. It may be necessary to make some renames and additions.


Open in Gitpod

…ebra.order.monoid_lemmas`

Although the names of these lemmas are from `algebra.order.monoid_lemmas`, many of the lemmas in `algebra.order.monoid_lemmas` have incorrect names, and some others may not be appropriate. Also, many lemmas are missing in `algebra.order.monoid_lemmas`. It may be necessary to make some renames and additions.
@negiizhao negiizhao added the awaiting-review The author would like community review of the PR label Mar 26, 2022
-- proven with `a0 : 0 ≤ a` as `mul_le_of_le_one_right'`
lemma mul_le_of_le_one_right [pos_mul_mono α] {a b : α} (h : b ≤ 1) (a0 : 0 < a) :
a * b ≤ a :=
mul_le_of_le_of_le_one le_rfl h a0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This name is from algebra.order.monoid_lemmas. But I'm wondering if it might be inappropriate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest opening a separate PR for this issue, in case you actually want to change the name of this lemma. Already as is, this PR is very long and will take time to review.

@negiizhao negiizhao requested a review from adomani March 26, 2022 19:14
@adomani
Copy link
Collaborator

adomani commented Apr 1, 2022

I'm leaving here a comment that I made privately to FR. This way, it stays in the PR trail.

One quick comment is that you mention the typeclass assumptions in each lemma, while most of the times, lemmas with the same typeclass assumptions are grouped together.

I'm not necessarily asking to change it, but I think that it is more standard to have sections with uniform typeclass assumptions.

I do not necessarily insist on this kind of grouping, but someone else might have a more conceptual preference.

@negiizhao
Copy link
Collaborator Author

My current approach is to sort by algebraic assumptions and order assumptions first, then put similar lemmas together. This approach might avoid missing lemmas, and one can figure out similar lemmas at the same time while reading. However, the grouped lemmas may not have the same co(ntra)variant typeclass assumptions. So (only) co(ntra)variant typeclass assumptions might be mentioned in each lemma.

@adomani
Copy link
Collaborator

adomani commented Apr 6, 2022

I am happy with either approach: sections with uniform typeclass assumptions, or similar looking lemmas grouped together. If someone else has a strong preference, they can say so.

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

bors merge

@leanprover-community-bot-assistant leanprover-community-bot-assistant added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Apr 9, 2022
bors bot pushed a commit that referenced this pull request Apr 9, 2022
…ebra.order.monoid_lemmas` (#12961)

Although the names of these lemmas are from `algebra.order.monoid_lemmas`, many of the lemmas in `algebra.order.monoid_lemmas` have incorrect names, and some others may not be appropriate. Also, many lemmas are missing in `algebra.order.monoid_lemmas`. It may be necessary to make some renames and additions.
@bors
Copy link

bors bot commented Apr 9, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Apr 9, 2022
…ebra.order.monoid_lemmas` (#12961)

Although the names of these lemmas are from `algebra.order.monoid_lemmas`, many of the lemmas in `algebra.order.monoid_lemmas` have incorrect names, and some others may not be appropriate. Also, many lemmas are missing in `algebra.order.monoid_lemmas`. It may be necessary to make some renames and additions.
@bors
Copy link

bors bot commented Apr 9, 2022

This PR was included in a batch that was canceled, it will be automatically retried

bors bot pushed a commit that referenced this pull request Apr 9, 2022
…ebra.order.monoid_lemmas` (#12961)

Although the names of these lemmas are from `algebra.order.monoid_lemmas`, many of the lemmas in `algebra.order.monoid_lemmas` have incorrect names, and some others may not be appropriate. Also, many lemmas are missing in `algebra.order.monoid_lemmas`. It may be necessary to make some renames and additions.
@bors
Copy link

bors bot commented Apr 9, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Apr 10, 2022
…ebra.order.monoid_lemmas` (#12961)

Although the names of these lemmas are from `algebra.order.monoid_lemmas`, many of the lemmas in `algebra.order.monoid_lemmas` have incorrect names, and some others may not be appropriate. Also, many lemmas are missing in `algebra.order.monoid_lemmas`. It may be necessary to make some renames and additions.
@bors
Copy link

bors bot commented Apr 10, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Apr 10, 2022
…ebra.order.monoid_lemmas` (#12961)

Although the names of these lemmas are from `algebra.order.monoid_lemmas`, many of the lemmas in `algebra.order.monoid_lemmas` have incorrect names, and some others may not be appropriate. Also, many lemmas are missing in `algebra.order.monoid_lemmas`. It may be necessary to make some renames and additions.
@bors
Copy link

bors bot commented Apr 10, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(algebra/order/monoid_lemmas_zero_lt): port some lemmas from algebra.order.monoid_lemmas [Merged by Bors] - feat(algebra/order/monoid_lemmas_zero_lt): port some lemmas from algebra.order.monoid_lemmas Apr 10, 2022
@bors bors bot closed this Apr 10, 2022
@bors bors bot deleted the FR_order_refactor branch April 10, 2022 08:34
bors bot pushed a commit that referenced this pull request Sep 14, 2022
…16447)

`pos_mul_mono`/`mul_pos_mono` and `pos_mul_reflect_lt`/`mul_pos_reflect_lt` were stated as covariance/contravariance of `{x : α // 0 < α}` over `α` even though the extension to `{x : α // 0 ≤ α}` also holds.

This meant that many lemmas were relying on antisymmetry only to treat the cases `a = 0` and `0 < a` separately, which made them need `partial_order` and depend on `classical.choice`. This prevented #16172 from actually removing the `decidable` lemma in `algebra.order.ring` after #16189 got merged.

Further, #16189 did not actually get rid of the temporary `zero_lt` namespace, so name conflicts that arise were not fixed.

Finally, `le_mul_of_one_le_left` and its seven friends were reproved inline about five time each.

Hence in this PR we
* restate `pos_mul_mono`, `mul_pos_mono`, `pos_mul_reflect_lt`, `mul_pos_reflect_lt` using `{x : α // 0 ≤ α}`
* provide the (classical) equivalence with the previous definitions in a `partial_order`.
* generalise lemmas from `partial_order` to `preorder`.
* delete all lemmas in the `preorder` namespace as they are now fully generalized. This in essence reverts (parts of) #12961, #13296 and #13299.
* replace most of the various `has_le`/`has_lt` assumptions by a blank `preorder` one, hence simplifying the file sectioning
* remove the `zero_lt` namespace.
* rename lemmas to fix name conflicts.
* delete a few lemmas that were left in `algebra.order.ring`.
* golf proofs involving `le_mul_of_one_le_left` and its seven friends. This is why the PR has a -450 lines diff.
b-mehta pushed a commit that referenced this pull request Sep 21, 2022
…16447)

`pos_mul_mono`/`mul_pos_mono` and `pos_mul_reflect_lt`/`mul_pos_reflect_lt` were stated as covariance/contravariance of `{x : α // 0 < α}` over `α` even though the extension to `{x : α // 0 ≤ α}` also holds.

This meant that many lemmas were relying on antisymmetry only to treat the cases `a = 0` and `0 < a` separately, which made them need `partial_order` and depend on `classical.choice`. This prevented #16172 from actually removing the `decidable` lemma in `algebra.order.ring` after #16189 got merged.

Further, #16189 did not actually get rid of the temporary `zero_lt` namespace, so name conflicts that arise were not fixed.

Finally, `le_mul_of_one_le_left` and its seven friends were reproved inline about five time each.

Hence in this PR we
* restate `pos_mul_mono`, `mul_pos_mono`, `pos_mul_reflect_lt`, `mul_pos_reflect_lt` using `{x : α // 0 ≤ α}`
* provide the (classical) equivalence with the previous definitions in a `partial_order`.
* generalise lemmas from `partial_order` to `preorder`.
* delete all lemmas in the `preorder` namespace as they are now fully generalized. This in essence reverts (parts of) #12961, #13296 and #13299.
* replace most of the various `has_le`/`has_lt` assumptions by a blank `preorder` one, hence simplifying the file sectioning
* remove the `zero_lt` namespace.
* rename lemmas to fix name conflicts.
* delete a few lemmas that were left in `algebra.order.ring`.
* golf proofs involving `le_mul_of_one_le_left` and its seven friends. This is why the PR has a -450 lines diff.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants