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

feat: more sub lemmas for Nat #203

Merged
merged 3 commits into from
Nov 24, 2023
Merged

Conversation

fgdorais
Copy link
Collaborator

@fgdorais fgdorais commented Jul 28, 2023

There's a bunch of deprecated lemmas, all of which have better alternatives except for one which appears to be a single purpose lemma that lost its purpose.

@semorrison semorrison added the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Aug 21, 2023
@semorrison
Copy link
Collaborator

@YaelDillies, do you have an opinion here, or are able to review?

@fgdorais
Copy link
Collaborator Author

This is the last in a series. They're mostly independent but it might be easier to start with #194.

@fgdorais fgdorais added awaiting-author Waiting for PR author to address issues and removed awaiting-review This PR is ready for review; the author thinks it is ready to be merged. labels Aug 21, 2023
Copy link

@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.

I'm happy with the deprecations. However I find it a bit sad to duplicate all this tsub material in Std. Can we do some subset of the following suggestions?

  • Not duplicate tsub lemma in Std
  • Put in the docstring of duplicate Nat.sub_something that tsub_something exists in mathlib
  • Deprecate mathlib-wise (is that currently possible?) each Nat.sub_something in favor of tsub_something
  • Put a big warning in Std.Data.Nat.Lemmas that if people want a lot of theory about truncated subtraction, then they should use Mathlib.Algebra.Order.Sub.Canonical instead of PRing more lemmas here

@YaelDillies YaelDillies mentioned this pull request Aug 30, 2023
1 task
@digama0
Copy link
Member

digama0 commented Aug 30, 2023

I'd rather we did none of the above. The tsub lemmas are more complex to understand, and elaborate. I think there should be no particular preference in mathlib for one over the other. Certainly there won't be a warning about not adding any more lemmas on Nat: tsub is only relevant if you care about other types with truncated subtraction like NNReal, and for a lot of users of Std that's just not relevant, while Nat is very relevant.

@YaelDillies
Copy link

YaelDillies commented Aug 30, 2023

If we had to only do one of my suggestions, I would like it to be

  • Put in the docstring of duplicate Nat.sub_something that tsub_something exists in mathlib

because I want mathlib users to be directed towards the API that's useful to them, avoiding ad hoc Std lemmas.

@fgdorais
Copy link
Collaborator Author

@YaelDillies Note that Nat lemmas in Std are primarily to reason about sizes of objects, array indices and such. There's no intent to duplicate or clash with Mathlib. My impression is that most users would use these lemmas for their intended purpose, so I don't quite see the need for the first three suggestions.

However, I am happy with the fourth suggestion:

  • Put a big warning in Std.Data.Nat.Lemmas that if people want a lot of theory about truncated subtraction, then they should use Mathlib.Algebra.Order.Sub.Canonical instead of PRing more lemmas here

I will add a module doc to clarify the intended purpose of these lemmas and a pointer to Mathlib alternatives.

I would like to hear what others think before implementing the first three suggestions.

@fgdorais fgdorais mentioned this pull request Sep 2, 2023
@fgdorais
Copy link
Collaborator Author

fgdorais commented Sep 2, 2023

@YaelDillies Please see #237.

@semorrison
Copy link
Collaborator

I agree that module-docs are the way to go to explain the additional theory available in Mathlib.

Any results that are needed or helpful for non-mathematical uses of Nat need to be here in Std, without expecting such users to know about the general theory of truncated subtraction.

I am ambivalent about the "Mathlib-specific deprecation" suggestion, but don't think we should mention tsub in Std.

@fgdorais, as far as I'm concerned I think you can mark this back as awaiting-review.

@fgdorais fgdorais added awaiting-review This PR is ready for review; the author thinks it is ready to be merged. and removed awaiting-author Waiting for PR author to address issues labels Sep 4, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Oct 19, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Nov 1, 2023
Std/Data/Nat/Lemmas.lean Outdated Show resolved Hide resolved
Std/Data/Nat/Lemmas.lean Outdated Show resolved Hide resolved
Copy link
Member

@digama0 digama0 left a comment

Choose a reason for hiding this comment

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

Approved with modification:

  • succ_sub_one, sub_lt_self, sub_one_sub_lt were undeprecated
  • deprecated theorems were moved after their respective undeprecated versions instead of in a separate section (if we want this it should be implemented in docgen)

fgdorais and others added 2 commits November 4, 2023 17:56
Potentially breaking:

* Changed parameters in `Nat.sub_le_sub_left`
* Deprecated `Nat.add_le_to_le_sub`
* Deprecated `Nat.succ_sub_one`
* Deprecated `Nat.le_of_le_of_sub_le_sub_left`
* Deprecated `Nat.le_of_le_of_sub_le_sub_right`
* Deprecated `Nat.add_le_of_le_sub_left`
* Deprecated `Nat.sub_lt_self`
* Deprecated `Nat.sub_lt_succ`
* Deprecated `Nat.sub_one_sub_lt`
* Some implicit parameters may have been renamed or reordered
Copy link
Collaborator

@semorrison semorrison left a comment

Choose a reason for hiding this comment

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

Looks good to me. @digama0 has already approved, and @fgdorais has resolved the merge conflicts, so I hope we can merge this round!!

@digama0 digama0 merged commit 3044327 into leanprover-community:main Nov 24, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review This PR is ready for review; the author thinks it is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants