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

chore(algebra/order/ring/lemmas): remove useless lemmas, use namespace without_zero_le_one #16525

Closed
wants to merge 9 commits into from

Conversation

negiizhao
Copy link
Collaborator

@negiizhao negiizhao commented Sep 15, 2022

The fourth part of #16449

There are still some useless lemmas that were simply ported from algebra/order/monoid_lemmas, this PR removes them. "useless lemmas" are lemmas that just chain an existing lemma with an assumption and lemmas whose assumptions imply 1 ≤ 0. These lemmas were simply ported from algebra/order/monoid_lemmas.

Also, some lemmas have both assumptions like 1 < a 0 < a, this is because the file does not use type classes like zero_le_one_class currently. This PR puts these lemmas in the namespace without_zero_le_one.

mathlib4: leanprover-community/mathlib4#782


Open in Gitpod

…e namespace `without_zero_le_one`

The fourth part of #16449

There are still some useless lemmas which were simply ported from `algebra/order/monoid_lemmas`, this PR removes them.

Also some lemmas have both assumptions like `1 < a` `0 < a`, this is because the file does not use type classes like `zero_le_one_class` currently. So I put these lemmas in the namespace `without_zero_le_one`.
@negiizhao negiizhao added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. t-algebra Algebra (groups, rings, fields etc) t-order Order hierarchy labels Sep 15, 2022
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Sep 16, 2022
@YaelDillies YaelDillies added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 8, 2022
@negiizhao negiizhao removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 10, 2022
Comment on lines 550 to 557
lemma mul_le_of_le_one_left [mul_pos_mono α] (hb : 0 ≤ b) (h : a ≤ 1) : a * b ≤ b :=
by simpa only [one_mul] using mul_le_mul_of_nonneg_right h hb
lemma mul_le_of_le_one_right [pos_mul_mono α] (a0 : 0 ≤ a) (h : b ≤ 1) : a * b ≤ a :=
by simpa only [mul_one] using mul_le_mul_of_nonneg_left h a0

lemma le_mul_of_one_le_left [mul_pos_mono α] (hb : 0 ≤ b) (h : 1 ≤ a) : b ≤ a * b :=
by simpa only [one_mul] using mul_le_mul_of_nonneg_right h hb
lemma le_mul_of_one_le_right [pos_mul_mono α] (a0 : 0 ≤ a) (h : 1 ≤ b) : a ≤ a * b :=
by simpa only [mul_one] using mul_le_mul_of_nonneg_left h a0

lemma mul_le_of_le_one_right [pos_mul_mono α] (ha : 0 ≤ a) (h : b ≤ 1) : a * b ≤ a :=
by simpa only [mul_one] using mul_le_mul_of_nonneg_left h ha
lemma mul_le_of_le_one_left [mul_pos_mono α] (b0 : 0 ≤ b) (h : a ≤ 1) : a * b ≤ b :=
by simpa only [one_mul] using mul_le_mul_of_nonneg_right h b0
Copy link
Member

Choose a reason for hiding this comment

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

Why did you swap the orders of these? It just makes the diff a lot bigger.

Copy link
Member

Choose a reason for hiding this comment

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

(also as a LTR language, I would expect the left lemmas to come before the right lemmas anyway)

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 order is consistent with the previous lemmas in the file. Also the order of their typeclass assumptions is consistent with the conventions in this file.

@negiizhao negiizhao changed the title chore(algebra/order/monoid_lemmas_zero_lt): remove useless lemmas, use namespace without_zero_le_one chore(algebra/order/ring/lemmas): remove useless lemmas, use namespace without_zero_le_one Nov 8, 2022
@semorrison
Copy link
Collaborator

Do we actually need the lemmas in without_zero_le_one? It seems they aren't used anywhere; this might be chance to just dump them?

@negiizhao
Copy link
Collaborator Author

negiizhao commented Nov 11, 2022

These can be dumped currently, but I'm thinking of strengthening these lemmas by using zero_le_one_class and replacing lemmas in algebra/order/ring/defs in the future.

@YaelDillies YaelDillies added awaiting-author A reviewer has asked the author a question or requested changes merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed awaiting-review The author would like community review of the PR labels Jan 7, 2023
@negiizhao negiizhao 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 merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Jan 7, 2023
@YaelDillies
Copy link
Collaborator

Could you explain what your definition of "useless lemma" is in the PR description and either

  • delete the lemmas in the without_zero_le_one namespace
  • add a zero_le_one_class assumption to each of them and instead delete the ordered ring lemmas and give an example where the generalised lemmas would be useful

@negiizhao
Copy link
Collaborator Author

"useless lemmas" is what you mentioned in doc-string and lemmas whose assumptions imply 1 ≤ 0.

Yaël: What's the point of these lemmas? They just chain an existing lemma with an assumption in
all possible ways, thereby artificially inflating the API and making the truly relevant lemmas hard
to find

These lemmas were simply ported from algebra/order/monoid_lemmas as I mentioned before.

I made PR #18158 to add zero_le_one_class assumption to each of them. #18157 shows ereal would have some covariance typeclasses and zero_le_one_class, but it has no semiring structure.

@YaelDillies
Copy link
Collaborator

I know what I meant. I'm just asking you to let everyone else know by putting it in the PR description 😄

@eric-wieser eric-wieser added the not-too-late This PR was ready at the point mathlib3 was frozen: we will try to merge it and port it to mathlib4 label Jul 15, 2023
@riccardobrasca
Copy link
Member

Can you please fix the conflict?

@semorrison semorrison 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 28, 2023
@negiizhao negiizhao added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jul 28, 2023
@github-actions github-actions bot added modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. and removed awaiting-CI The author would like to see what CI has to say before doing more work. labels Jul 28, 2023
@semorrison
Copy link
Collaborator

@YaelDillies, could you comment on this PR? What remains to be done?

@eric-wieser
Copy link
Member

Closing, since this was ported to leanprover-community/mathlib4#9250

@YaelDillies YaelDillies removed awaiting-review The author would like community review of the PR not-too-late This PR was ready at the point mathlib3 was frozen: we will try to merge it and port it to mathlib4 labels Mar 24, 2024
@YaelDillies YaelDillies removed their request for review March 24, 2024 08:39
@YaelDillies YaelDillies deleted the FR_order_refactor18 branch March 24, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. t-algebra Algebra (groups, rings, fields etc) t-order Order hierarchy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants