Skip to content

[Merged by Bors] - refactor: do not allow qsmul to default automatically#11262

Closed
eric-wieser wants to merge 7 commits intomasterfrom
eric-wieser/remove-qsmulDefault
Closed

[Merged by Bors] - refactor: do not allow qsmul to default automatically#11262
eric-wieser wants to merge 7 commits intomasterfrom
eric-wieser/remove-qsmulDefault

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.


Open in Gitpod

@eric-wieser eric-wieser added awaiting-review awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. t-algebra Algebra (groups, rings, fields, etc) labels Mar 9, 2024
@eric-wieser eric-wieser requested a review from mattrobball March 9, 2024 12:14
@github-actions github-actions bot removed the awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. label Mar 9, 2024
@mattrobball
Copy link
Copy Markdown
Contributor

!bench

mul_inv_cancel x := CauSeq.Completion.mul_inv_cancel
ratCast q := ofRat q
ratCast_mk n d hd hnd := by rw [← ofRat_ratCast, Rat.cast_mk', ofRat_mul, ofRat_inv]; rfl
qsmul := qsmulRec _ -- TODO: fix instance diamond
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe make a separate issue or add to an existing one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Easier to just fix it, #11263

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit cb4d250.
There were significant changes against commit 25c5340:

  Benchmark                            Metric         Change
  ==========================================================
+ build                                linting         -5.6%
+ ~Mathlib.RingTheory.IntegralDomain   instructions   -39.7%

can be found in `Mathlib.RingTheory.LittleWedderburn`. -/
def Fintype.fieldOfDomain (R) [CommRing R] [IsDomain R] [DecidableEq R] [Fintype R] : Field R :=
{ Fintype.groupWithZeroOfCancel R, ‹CommRing R› with }
{ Fintype.divisionRingOfIsDomain R, ‹CommRing R› with }
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess this caused the speedup?

@mattrobball
Copy link
Copy Markdown
Contributor

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Mar 9, 2024
mathlib-bors bot pushed a commit that referenced this pull request Mar 9, 2024
Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.
@YaelDillies
Copy link
Copy Markdown
Contributor

Is that PR the start of the doom of Type-valued default fields?

@eric-wieser
Copy link
Copy Markdown
Member Author

For now I think it's only the ones involved in diamonds that matter. There'd be no benefit to changing sub and div.

@eric-wieser
Copy link
Copy Markdown
Member Author

pow and zpow are probably also not worth the trouble; but nat/int/ratCast are next in line.

@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Mar 9, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor: do not allow qsmul to default automatically [Merged by Bors] - refactor: do not allow qsmul to default automatically Mar 9, 2024
@mathlib-bors mathlib-bors bot closed this Mar 9, 2024
@mathlib-bors mathlib-bors bot deleted the eric-wieser/remove-qsmulDefault branch March 9, 2024 18:26
kbuzzard pushed a commit that referenced this pull request Mar 12, 2024
Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.
utensil pushed a commit that referenced this pull request Mar 26, 2024
Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.
xgenereux pushed a commit that referenced this pull request Apr 15, 2024
Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.
uniwuni pushed a commit that referenced this pull request Apr 19, 2024
Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.
callesonne pushed a commit that referenced this pull request Apr 22, 2024
Follows on from #6262.
Again, this does not attempt to fix any diamonds; it only identifies where they may be.
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. t-algebra Algebra (groups, rings, fields, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants