-
Notifications
You must be signed in to change notification settings - Fork 316
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: add a FloorSemiring instance to NNRat #13548
base: master
Are you sure you want to change the base?
Conversation
This PR/issue depends on:
|
PR summary 6d6f59bbacImport changes for modified filesNo significant changes to the import graph Import changes for all files
|
|
||
We define the `FloorSemiring` instance on `ℚ≥0`, and relate it's operators to `NNRat.cast`. | ||
|
||
Note that we cannot talk about `Int.fract`, which currently only works for rings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't that mean we are missing Nat.fract
with the obvious definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but adding new definitions seems out of scope for a PR that adds new instances.
Mathlib/Data/Rat/Cast/Order.lean
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the same lemmas comparing Nat.cast
and Int.cast
? (I think they are also missing?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to declare this out of scope for a PR about NNRat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
maintainer merge
🚀 Pull request has been placed on the maintainer queue by YaelDillies. |
Otherwise LGTM. Thanks! |
✌️ eric-wieser can now approve this pull request. To approve and merge a pull request, simply reply with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Also some missing `norm_cast` lemmas for inequalities of casts between different types.
Build failed (retrying...): |
bors r- |
Canceled. |
Can you merge master, fix the build, and send it back to bors? |
✌️ eric-wieser can now approve this pull request. To approve and merge a pull request, simply reply with |
Also some missing
norm_cast
lemmas for inequalities of casts between different types.