Skip to content

[Merged by Bors] - feat: NNRat.cast#11203

Closed
YaelDillies wants to merge 8 commits intomasterfrom
nnrat_cast
Closed

[Merged by Bors] - feat: NNRat.cast#11203
YaelDillies wants to merge 8 commits intomasterfrom
nnrat_cast

Conversation

@YaelDillies YaelDillies added WIP Work in progress blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) t-algebra Algebra (groups, rings, fields, etc) labels Mar 6, 2024
@ghost ghost removed the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Mar 7, 2024
@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 8, 2024
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 8, 2024
@ghost ghost added merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) labels Mar 9, 2024
@ghost ghost added blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) labels Mar 17, 2024
@ghost ghost added merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) and removed blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) labels Mar 18, 2024
YaelDillies added a commit that referenced this pull request Mar 18, 2024
@ghost ghost added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Mar 18, 2024
YaelDillies added a commit that referenced this pull request Mar 19, 2024
YaelDillies added a commit that referenced this pull request Mar 19, 2024
These lemmas are needed to define the semifield structure on `NNRat`, hence I am repurposing `Algebra.Order.Field.Defs` from avoiding a timeout (which I believe was solved long ago) to avoiding to import random stuff in the definition of the semifield structure on `NNRat` (although this PR doesn't actually reduce imports there, it will be in a later PR).

Reduce the diff of #11203
YaelDillies added a commit that referenced this pull request Mar 19, 2024
xgenereux pushed a commit that referenced this pull request Apr 15, 2024
xgenereux pushed a commit that referenced this pull request Apr 15, 2024
by declaring them all in `where` style with implicit type assumptions and `inst` prefix

Here to reduce the diff from #11203
xgenereux pushed a commit that referenced this pull request Apr 15, 2024
Among other things, change the `nsmul`, `zsmul`, `qsmul` fields to have `n`/`q` come before `x`, because this matches the lemmas we want to write about them. It would be preferrable to perform the same changes to the `AddMonoid`/`AddGroup`-like typeclasses, but this is impossible with the current `to_additive` framework, so instead I have inserted some `Function.swap` at the interface between `AddMonoid`/`AddGroup` and `Ring`/`Field`.

Reduce the diff of #11203
xgenereux pushed a commit that referenced this pull request Apr 15, 2024
Soon, there will be `NNRat` analogs of the `Rat` fields in the definition of `Field`. `NNRat` is less nicely a structure than `Rat`, hence there is a need to reduce the dependency of `Field` on the internals of `Rat`.

This PR achieves this by restating `Field.ratCast_mk'` in terms of `Rat.num`, `Rat.den`. This requires fixing a few downstream instances.

Reduce the diff of #11203.




Co-authored-by: Floris van Doorn <fpvdoorn@gmail.com>
xgenereux pushed a commit that referenced this pull request Apr 15, 2024
…atCast` (#11499)

This is less exhaustive than its sibling #11486 because edge cases are harder to classify. No fundamental difficulty, just me being a bit fast and lazy.

Reduce the diff of #11203
mathlib-bors bot pushed a commit that referenced this pull request Apr 15, 2024
`Rat` has a long history in (and before) mathlib and as such its development is full of cruft. Now that `NNRat` is a thing, there is a need for the theory of `Rat` to be mimickable to yield the theory of `NNRat`, which is not currently the case.

Broadly, this PR aims at mirroring the `Rat` and `NNRat` declarations. It achieves this by:
* Relying more on `Rat.num` and `Rat.den`, and less on the structure representation of `Rat`
* Abandoning the vestigial `Rat.Nonneg` (which was replaced in Std by a new development of the order on `Rat`)
* Renaming many `Rat` lemmas with dubious names. This creates quite a lot of conflicts with Std lemmas, whose names are themselves dubious. I am priming the relevant new mathlib names and leaving TODOs to fix the Std names.
* Handling most of the `Rat` porting notes

Reduce the diff of #11203
Comment on lines +29 to +32
## Huge warning

Whenever you state a lemma about the coercion `ℚ≥0 → ℚ`, check that Lean inserts `NNRat.cast`, not
`Subtype.val`. Else your lemma will never apply.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ouch, this is rather unfortunate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah... The precise issue is that ⟨q, hq⟩ notation creates a term of type {q : ℚ // 0 ≤ q}, not ℚ≥0, so it finds Subtype.val as the coercion, not NNRat.cast

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This suggests we might want an NNRat.mk that has the correct return type?

But if so, that can wait until a later PR

bors merge

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but it won't have the constructor notation. To properly solve it we should either:

  • make NNRat reducible
  • make NNRat a one-field structure
  • make NNRat a fully fledged structure with num den : Nat etc

Define the canonical coercion from the nonnegative rationals to any division semiring.

From LeanAPAP
@sgouezel
Copy link
Copy Markdown
Contributor

!bench

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit f76c7be.
There were significant changes against commit e082274:

  Benchmark                                          Metric         Change
  ========================================================================
- ~Mathlib.FieldTheory.Subfield                      instructions    15.8%
- ~Mathlib.LinearAlgebra.Matrix.SpecialLinearGroup   instructions     7.7%

@sgouezel
Copy link
Copy Markdown
Contributor

The bench is very good: I was afraid of a global slowdown, but there is nothing in sight. I don't think the slowdown in two specific files has any importance (although if you can understand what is going on this would be even better).

@YaelDillies
Copy link
Copy Markdown
Contributor Author

Yes, I already removed the problematic instances like Module ℚ α → Module ℚ≥0 α.

For FieldTheory.Subfield, I just checked and it's indeed simply due to the DivisionRing instance having more data. For LinearAlgebra.Matrix.SpecialLinearGroup, nothing is sticking out as suspicious to me.

Copy link
Copy Markdown
Contributor

@Ruben-VandeVelde Ruben-VandeVelde left a comment

Choose a reason for hiding this comment

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

Code looks fine, assuming this is a change we want.

maintainer merge

@github-actions
Copy link
Copy Markdown

🚀 Pull request has been placed on the maintainer queue by Ruben-VandeVelde.

@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Apr 24, 2024

Pull request successfully merged into master.

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. t-algebra Algebra (groups, rings, fields, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants