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(NumberTheory/WellApproximable): golf proof using tauto #5263

Closed
wants to merge 2 commits into from

Conversation

ocfnash
Copy link
Contributor

@ocfnash ocfnash commented Jun 19, 2023

Two independent changes are being proposed here:

  1. Using tauto: this was annoyingly slow in Lean 3 but is not in Lean 4,
  2. Changing p to ((p : ℕ) : ℝ): this is because Lean 4 is unfortunately much worse at coercing from Nat.Primes to Real and will waste about 5 seconds working on this via typeclass inference of CoeT searches without this change.

Some discussion here on Zulip.


Open in Gitpod

Using `tauto` here was annoyingly slow in Lean 3 but is not in Lean 4.

On the other hand, the Lean 4 proof is much slower overall than Lean 3.
Using `set_option profiler true` this seems to be a problem with
coercions since I see 20 typeclass searches as follows:
```
typeclass inference of CoeT took 384ms
typeclass inference of CoeT took 368ms
typeclass inference of CoeT took 328ms
typeclass inference of CoeT took 306ms
typeclass inference of CoeT took 307ms
typeclass inference of CoeT took 309ms
typeclass inference of CoeT took 307ms
typeclass inference of CoeT took 308ms
typeclass inference of CoeT took 335ms
typeclass inference of CoeT took 307ms
typeclass inference of CoeT took 307ms
typeclass inference of CoeT took 309ms
typeclass inference of CoeT took 307ms
typeclass inference of CoeT took 308ms
typeclass inference of CoeT took 306ms
typeclass inference of CoeT took 306ms
typeclass inference of CoeT took 312ms
typeclass inference of CoeT took 324ms
typeclass inference of CoeT took 319ms
typeclass inference of CoeT took 312ms
```
(adding up to nearly 6.5 seconds).
@ocfnash ocfnash added awaiting-review easy < 20s of review time. See the lifecycle page for guidelines. labels Jun 19, 2023
This removes ~5 seconds of slow instance searches for `CoeT`.
@semorrison
Copy link
Contributor

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Jun 21, 2023
bors bot pushed a commit that referenced this pull request Jun 21, 2023
Two independent changes are being proposed here:
 1. Using `tauto`: this was annoyingly slow in Lean 3 but is not in Lean 4,
 2. Changing `p` to `((p : ℕ) : ℝ)`: this is because Lean 4 is unfortunately much worse at coercing from `Nat.Primes` to `Real` and will waste about 5 seconds working on this via `typeclass inference of CoeT` searches without this change.

Some discussion [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Slow.20coercions) on Zulip.
@bors
Copy link

bors bot commented Jun 21, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title chore(NumberTheory/WellApproximable): golf proof using tauto [Merged by Bors] - chore(NumberTheory/WellApproximable): golf proof using tauto Jun 21, 2023
@bors bors bot closed this Jun 21, 2023
@bors bors bot deleted the ocfnash/well_approx_tauto branch June 21, 2023 05:19
alexkeizer pushed a commit that referenced this pull request Jun 22, 2023
Two independent changes are being proposed here:
 1. Using `tauto`: this was annoyingly slow in Lean 3 but is not in Lean 4,
 2. Changing `p` to `((p : ℕ) : ℝ)`: this is because Lean 4 is unfortunately much worse at coercing from `Nat.Primes` to `Real` and will waste about 5 seconds working on this via `typeclass inference of CoeT` searches without this change.

Some discussion [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Slow.20coercions) on Zulip.
semorrison pushed a commit that referenced this pull request Jun 23, 2023
Two independent changes are being proposed here:
 1. Using `tauto`: this was annoyingly slow in Lean 3 but is not in Lean 4,
 2. Changing `p` to `((p : ℕ) : ℝ)`: this is because Lean 4 is unfortunately much worse at coercing from `Nat.Primes` to `Real` and will waste about 5 seconds working on this via `typeclass inference of CoeT` searches without this change.

Some discussion [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Slow.20coercions) on Zulip.
semorrison pushed a commit that referenced this pull request Jun 23, 2023
Two independent changes are being proposed here:
 1. Using `tauto`: this was annoyingly slow in Lean 3 but is not in Lean 4,
 2. Changing `p` to `((p : ℕ) : ℝ)`: this is because Lean 4 is unfortunately much worse at coercing from `Nat.Primes` to `Real` and will waste about 5 seconds working on this via `typeclass inference of CoeT` searches without this change.

Some discussion [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Slow.20coercions) on Zulip.
semorrison pushed a commit that referenced this pull request Jun 25, 2023
Two independent changes are being proposed here:
 1. Using `tauto`: this was annoyingly slow in Lean 3 but is not in Lean 4,
 2. Changing `p` to `((p : ℕ) : ℝ)`: this is because Lean 4 is unfortunately much worse at coercing from `Nat.Primes` to `Real` and will waste about 5 seconds working on this via `typeclass inference of CoeT` searches without this change.

Some discussion [here](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Slow.20coercions) on Zulip.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy < 20s of review time. See the lifecycle page for guidelines. 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

2 participants