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: port Data.Nat.PSub #806

Closed
wants to merge 7 commits into from
Closed

Conversation

zeramorphic
Copy link
Collaborator

mathlib3 SHA: 62a5626868683c104774de8d85b9855234ac807c

I'm not sure if the file should be called Psub.lean or PSub.lean - mathport chooses the former.

Signed-off-by: thirdsgames <[email protected]>
Signed-off-by: thirdsgames <[email protected]>
@zeramorphic zeramorphic added awaiting-review mathlib-port This is a port of a theory file from mathlib. labels Nov 30, 2022
@kim-em
Copy link
Contributor

kim-em commented Nov 30, 2022

Please rename to PSub.

/-!
# Partial predecessor and partial subtraction on the natural numbers

The usual definition of natural number subtraction (`nat.sub`) returns 0 as a "garbage value" for
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The usual definition of natural number subtraction (`nat.sub`) returns 0 as a "garbage value" for
The usual definition of natural number subtraction (`Nat.sub`) returns 0 as a "garbage value" for

Signed-off-by: thirdsgames <[email protected]>
# Partial predecessor and partial subtraction on the natural numbers

The usual definition of natural number subtraction (`nat.sub`) returns 0 as a "garbage value" for
`a - b` when `a < b`. Similarly, `nat.pred 0` is defined to be `0`. The functions in this file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`a - b` when `a < b`. Similarly, `nat.pred 0` is defined to be `0`. The functions in this file
`a - b` when `a < b`. Similarly, `Nat.pred 0` is defined to be `0`. The functions in this file

Comment on lines 18 to 19
- `nat.ppred`: a partial predecessor operation
- `nat.psub`: a partial subtraction operation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `nat.ppred`: a partial predecessor operation
- `nat.psub`: a partial subtraction operation
- `Nat.ppred`: a partial predecessor operation
- `Nat.psub`: a partial subtraction operation

Mathlib/Data/Nat/PSub.lean Outdated Show resolved Hide resolved

/-- Partial predecessor operation. Returns `ppred n = some m`
if `n = m + 1`, otherwise `none`. -/
@[simp]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove @[simp] here and on psub. You may need to write the simp lemmas for the branches yourself.

Putting @[simp] on a definition by matching has changed behaviour in Lean 4.

| n + 1 => by constructor <;> intro <;> contradiction
#align nat.ppred_eq_none Nat.ppred_eq_none

-- Porting note: `simp` eagerly expanded `psub` and `ppred` so it couldn't apply the `simp` lemmas
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Comment on lines 99 to 101
psub m (n + k) = (do
let x ← psub m n
psub x k) :=
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
psub m (n + k) = (do
let x ← psub m n
psub x k) :=
psub m (n + k) = (do psub (← psub m n) k) :=

Comment on lines 42 to 43
-- Porting note: mathport failed to align `option.get_or_else` with `Option.getD`

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- Porting note: mathport failed to align `option.get_or_else` with `Option.getD`

@kim-em kim-em changed the title feat: port Data.Nat.Psub feat: port Data.Nat.PSub Nov 30, 2022
@zeramorphic
Copy link
Collaborator Author

Thanks for your comments, I think I've made all of the relevant changes.

Signed-off-by: thirdsgames <[email protected]>
@kim-em
Copy link
Contributor

kim-em commented Nov 30, 2022

Great, thanks. It's helpful if in mathlib PR reviews, if the author can click "resolve conversation" on everything they've dealt with. Because we have multiple reviewers dealing with things, it's great if someone coming in can quickly see what is and isn't done.

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Nov 30, 2022
bors bot pushed a commit that referenced this pull request Nov 30, 2022
mathlib3 SHA: 62a5626868683c104774de8d85b9855234ac807c

I'm not sure if the file should be called `Psub.lean` or `PSub.lean` - mathport chooses the former.

Co-authored-by: zeramorphic <[email protected]>
@zeramorphic
Copy link
Collaborator Author

That makes sense, thanks - I'm not very used to collaborating on projects like this just yet.

@bors
Copy link

bors bot commented Nov 30, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: port Data.Nat.PSub [Merged by Bors] - feat: port Data.Nat.PSub Nov 30, 2022
@bors bors bot closed this Nov 30, 2022
@bors bors bot deleted the Data.Nat.Psub branch November 30, 2022 22:10
bors bot pushed a commit to leanprover-community/mathlib3 that referenced this pull request Dec 1, 2022
Regenerated from the [port status wiki page](https://github.com/leanprover-community/mathlib/wiki/mathlib4-port-status).
Relates to the following PRs:
* `algebra.field.defs`: leanprover-community/mathlib4#668
* `algebra.group.commute`: leanprover-community/mathlib4#750
* `algebra.group_with_zero.commute`: leanprover-community/mathlib4#762
* `algebra.group_with_zero.semiconj`: leanprover-community/mathlib4#757
* `algebra.group_with_zero.units.basic`: leanprover-community/mathlib4#735
* `algebra.hom.embedding`: leanprover-community/mathlib4#764
* `algebra.order.monoid.cancel.defs`: leanprover-community/mathlib4#774
* `algebra.order.monoid.canonical.defs`: leanprover-community/mathlib4#778
* `algebra.order.monoid.defs`: leanprover-community/mathlib4#771
* `algebra.order.monoid.min_max`: leanprover-community/mathlib4#763
* `algebra.order.monoid.order_dual`: leanprover-community/mathlib4#786
* `algebra.order.sub.defs`: leanprover-community/mathlib4#732
* `algebra.regular.basic`: leanprover-community/mathlib4#758
* `algebra.ring.commute`: leanprover-community/mathlib4#759
* `algebra.ring.regular`: leanprover-community/mathlib4#795
* `algebra.ring.semiconj`: leanprover-community/mathlib4#751
* `control.applicative`: leanprover-community/mathlib4#798
* `control.functor`: leanprover-community/mathlib4#612
* `control.monad.basic`: leanprover-community/mathlib4#752
* `data.countable.defs`: leanprover-community/mathlib4#736
* `data.int.units`: leanprover-community/mathlib4#807
* `data.nat.basic`: leanprover-community/mathlib4#729
* `data.nat.psub`: leanprover-community/mathlib4#806
* `data.nat.units`: leanprover-community/mathlib4#805
* `data.pi.algebra`: leanprover-community/mathlib4#564
* `data.prod.lex`: leanprover-community/mathlib4#783
* `logic.embedding.basic`: leanprover-community/mathlib4#700
* `logic.equiv.option`: leanprover-community/mathlib4#674
* `order.bounded_order`: leanprover-community/mathlib4#697
* `order.with_bot`: leanprover-community/mathlib4#776
* `order.disjoint`: leanprover-community/mathlib4#773
* `order.min_max`: leanprover-community/mathlib4#728
* `order.prop_instances`: leanprover-community/mathlib4#792
* `order.rel_iso.basic`: leanprover-community/mathlib4#772
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mathlib-port This is a port of a theory file from mathlib. ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants