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] - chore: make instSMulRealComplex scoped #12080

Closed
wants to merge 7 commits into from

Conversation

MichaelStollBayreuth
Copy link
Collaborator

@MichaelStollBayreuth MichaelStollBayreuth commented Apr 12, 2024

This scopes and moves to Complex.SMul the instance instSMulRealComplex.
Rationale: This instance is used in Data.Complex.{Basic|Module} to bootstrap SMul instances from the reals, but afterwards, instances SMul R ℂ should not need to rely on that (rather be obtained via Algebra R ℂ, for example). This speeds up the two mentioned files a bit (in fact, it reverts a slow-down caused by reducing the instance priority in #12070) and does not seem to have any adverse effects.

I think this is a cleaner solution compared to just reducing the instance priority.

See here on Zulip.


Open in Gitpod

@MichaelStollBayreuth
Copy link
Collaborator Author

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 3fc1b49.
There were no significant changes against commit 0129c03.

@MichaelStollBayreuth
Copy link
Collaborator Author

There is a positive effect on Mathlib.Data.Complex.{Basic|Module} (and also overall):

Mathlib.Data.Complex.Basic instructions 30,567 Mrd. -1,919 Mrd. -6,278 % 28,648 Mrd.
Mathlib.Data.Complex.Module instructions 101,03 Mrd. -6,104 Mrd. -6,042 % 94,926 Mrd.
build instructions 111,658 Bio. -13,731 Mrd. -0,012 % 111,644 Bio.
(Mrd. = 10⁹, Bio. = 10¹²)

This is likely caused by the instance now being top of the pile in these files, where it is really needed (but is not present outside).

@MichaelStollBayreuth MichaelStollBayreuth added awaiting-review t-algebra Algebra (groups, rings, fields etc) and removed WIP Work in progress labels Apr 12, 2024
@MichaelStollBayreuth MichaelStollBayreuth changed the title chore: make instSMulRealComplex local chore: make instSMulRealComplex scoped Apr 12, 2024
@mattrobball
Copy link
Collaborator

Does scoping an instance actually do anything?

@MichaelStollBayreuth
Copy link
Collaborator Author

Does scoping an instance actually do anything?

Yes:

import Mathlib

variable (a b : ℂ)

#check a ≤ b -- failed to synthesize instance LE ℂ

open scoped ComplexOrder

#check a ≤ b -- a ≤ b : Prop

@mattrobball
Copy link
Collaborator

I had a previous experiment where I tried to retroactively scope some parent projections which didn't work. But the problem was the instance erasure and not the scoping.

I think scoped instances are a very good idea as another lever to guide typeclass synthesis.

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Apr 14, 2024
mathlib-bors bot pushed a commit that referenced this pull request Apr 14, 2024
This scopes and moves to `Complex.SMul` the instance `instSMulRealComplex`.
Rationale: This instance is used in `Data.Complex.{Basic|Module}` to bootstrap `SMul` instances from the reals, but afterwards, instances `SMul R ℂ` should not need to rely on that (rather be obtained via `Algebra R ℂ`, for example). This speeds up the two mentioned files a bit (in fact, it reverts a slow-down caused by reducing the instance priority in #12070) and does not seem to have any adverse effects.

I think this is a cleaner solution compared to just reducing the instance priority.

See [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/simp.20prefers.20CharP.2Ecast_eq_zero.20over.20Nat.2Ecast_zero/near/432855625) on Zulip.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Apr 14, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: make instSMulRealComplex scoped [Merged by Bors] - chore: make instSMulRealComplex scoped Apr 14, 2024
@mathlib-bors mathlib-bors bot closed this Apr 14, 2024
@mathlib-bors mathlib-bors bot deleted the MS_SMulRealComplex_to_def branch April 14, 2024 10:41
Louddy pushed a commit that referenced this pull request Apr 15, 2024
This scopes and moves to `Complex.SMul` the instance `instSMulRealComplex`.
Rationale: This instance is used in `Data.Complex.{Basic|Module}` to bootstrap `SMul` instances from the reals, but afterwards, instances `SMul R ℂ` should not need to rely on that (rather be obtained via `Algebra R ℂ`, for example). This speeds up the two mentioned files a bit (in fact, it reverts a slow-down caused by reducing the instance priority in #12070) and does not seem to have any adverse effects.

I think this is a cleaner solution compared to just reducing the instance priority.

See [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/simp.20prefers.20CharP.2Ecast_eq_zero.20over.20Nat.2Ecast_zero/near/432855625) on Zulip.
atarnoam pushed a commit that referenced this pull request Apr 16, 2024
This scopes and moves to `Complex.SMul` the instance `instSMulRealComplex`.
Rationale: This instance is used in `Data.Complex.{Basic|Module}` to bootstrap `SMul` instances from the reals, but afterwards, instances `SMul R ℂ` should not need to rely on that (rather be obtained via `Algebra R ℂ`, for example). This speeds up the two mentioned files a bit (in fact, it reverts a slow-down caused by reducing the instance priority in #12070) and does not seem to have any adverse effects.

I think this is a cleaner solution compared to just reducing the instance priority.

See [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/simp.20prefers.20CharP.2Ecast_eq_zero.20over.20Nat.2Ecast_zero/near/432855625) on Zulip.
uniwuni pushed a commit that referenced this pull request Apr 19, 2024
This scopes and moves to `Complex.SMul` the instance `instSMulRealComplex`.
Rationale: This instance is used in `Data.Complex.{Basic|Module}` to bootstrap `SMul` instances from the reals, but afterwards, instances `SMul R ℂ` should not need to rely on that (rather be obtained via `Algebra R ℂ`, for example). This speeds up the two mentioned files a bit (in fact, it reverts a slow-down caused by reducing the instance priority in #12070) and does not seem to have any adverse effects.

I think this is a cleaner solution compared to just reducing the instance priority.

See [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/simp.20prefers.20CharP.2Ecast_eq_zero.20over.20Nat.2Ecast_zero/near/432855625) on Zulip.
callesonne pushed a commit that referenced this pull request Apr 22, 2024
This scopes and moves to `Complex.SMul` the instance `instSMulRealComplex`.
Rationale: This instance is used in `Data.Complex.{Basic|Module}` to bootstrap `SMul` instances from the reals, but afterwards, instances `SMul R ℂ` should not need to rely on that (rather be obtained via `Algebra R ℂ`, for example). This speeds up the two mentioned files a bit (in fact, it reverts a slow-down caused by reducing the instance priority in #12070) and does not seem to have any adverse effects.

I think this is a cleaner solution compared to just reducing the instance priority.

See [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/simp.20prefers.20CharP.2Ecast_eq_zero.20over.20Nat.2Ecast_zero/near/432855625) on Zulip.
Jun2M pushed a commit that referenced this pull request Apr 24, 2024
This scopes and moves to `Complex.SMul` the instance `instSMulRealComplex`.
Rationale: This instance is used in `Data.Complex.{Basic|Module}` to bootstrap `SMul` instances from the reals, but afterwards, instances `SMul R ℂ` should not need to rely on that (rather be obtained via `Algebra R ℂ`, for example). This speeds up the two mentioned files a bit (in fact, it reverts a slow-down caused by reducing the instance priority in #12070) and does not seem to have any adverse effects.

I think this is a cleaner solution compared to just reducing the instance priority.

See [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/simp.20prefers.20CharP.2Ecast_eq_zero.20over.20Nat.2Ecast_zero/near/432855625) on Zulip.
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.

None yet

3 participants