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] - refactor(Data/Rat/NNRat): move module and algebra instances #9951

Closed
wants to merge 7 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jan 24, 2024

As with #9950, this is motivated by:

  • Getting NNRat closed to norm_num
  • Being able to put an nnrat_cast field in DivisionSemirings

This brings down the number of dependencies of NNRat by around 600.


Open in Gitpod

The graph is still a monster:

nnrat

We already have the instances for `ℕ`, `ℤ`, and `ℚ` in this file, so adding `NNRat` doesn't feel that out of place.
@eric-wieser eric-wieser added awaiting-author A reviewer has asked the author a question or requested changes awaiting-CI labels Jan 24, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added blocked-by-other-PR This PR depends on another PR which is still in the queue. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. labels Jan 24, 2024
@@ -3,9 +3,10 @@ Copyright (c) 2022 Yaël Dillies, Bhavik Mehta. All rights reserved.
Released under Apache 2.0 license as described in the file LICENSE.
Authors: Yaël Dillies, Bhavik Mehta
-/
import Mathlib.Algebra.Algebra.Basic
import Mathlib.Algebra.Function.Indicator
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this would remove another 30 or so dependencies, but I'm declaring that a problem for a follow-up. I think further reduction probably entails splitting this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that I already reduced the dependencies of Algebra.Function.Indicator by quite a lot because of this.

@eric-wieser eric-wieser 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 labels Jan 24, 2024
@YaelDillies
Copy link
Collaborator

I just noticed that Algebra.Order.Archimedean depends on LinearAlgebra.Basic through NNRat 😱

maintainer merge

Copy link

github-actions bot commented Feb 1, 2024

🚀 Pull request has been placed on the maintainer queue by YaelDillies.

@eric-wieser
Copy link
Member Author

I just noticed that Algebra.Order.Archimedean depends on LinearAlgebra.Basic through NNRat 😱

Only before this PR though, right?

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

bors merge

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Feb 1, 2024
@YaelDillies
Copy link
Collaborator

I just noticed that Algebra.Order.Archimedean depends on LinearAlgebra.Basic through NNRat 😱

Only before this PR though, right?

Actually, this spurious dependency was introduced in #9950.

mathlib-bors bot pushed a commit that referenced this pull request Feb 1, 2024
As with #9950, this is motivated by:
* Getting `NNRat` closed to `norm_num`
* Being able to put an `nnrat_cast` field in `DivisionSemiring`s

This brings down the number of dependencies of `NNRat` by around 600.
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 1, 2024

Build failed (retrying...):

mathlib-bors bot pushed a commit that referenced this pull request Feb 1, 2024
As with #9950, this is motivated by:
* Getting `NNRat` closed to `norm_num`
* Being able to put an `nnrat_cast` field in `DivisionSemiring`s

This brings down the number of dependencies of `NNRat` by around 600.
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 1, 2024

Build failed (retrying...):

  • Build

mathlib-bors bot pushed a commit that referenced this pull request Feb 1, 2024
As with #9950, this is motivated by:
* Getting `NNRat` closed to `norm_num`
* Being able to put an `nnrat_cast` field in `DivisionSemiring`s

This brings down the number of dependencies of `NNRat` by around 600.
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 1, 2024

Build failed:

@eric-wieser
Copy link
Member Author

@ocfnash, it seems your #10086 broke this; are you happy for me to move those lemmas to a new file?

@ocfnash
Copy link
Contributor

ocfnash commented Feb 1, 2024

@ocfnash, it seems your #10086 broke this; are you happy for me to move those lemmas to a new file?

Yes of course. Do you need any help? Which lemmas would you like to move?

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added blocked-by-other-PR This PR depends on another PR which is still in the queue. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. labels Feb 1, 2024
@eric-wieser
Copy link
Member Author

bors merge

mathlib-bors bot pushed a commit that referenced this pull request Feb 1, 2024
As with #9950, this is motivated by:
* Getting `NNRat` closed to `norm_num`
* Being able to put an `nnrat_cast` field in `DivisionSemiring`s

This brings down the number of dependencies of `NNRat` by around 600.
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 1, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor(Data/Rat/NNRat): move module and algebra instances [Merged by Bors] - refactor(Data/Rat/NNRat): move module and algebra instances Feb 1, 2024
@mathlib-bors mathlib-bors bot closed this Feb 1, 2024
@mathlib-bors mathlib-bors bot deleted the eric-wieser/move-nnrat-module branch February 1, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants