Skip to content

[Merged by Bors] - feat: port Data.Nat.PrimeNormNum#3892

Closed
fpvandoorn wants to merge 23 commits intomasterfrom
port/Data.Nat.PrimeNormNum
Closed

[Merged by Bors] - feat: port Data.Nat.PrimeNormNum#3892
fpvandoorn wants to merge 23 commits intomasterfrom
port/Data.Nat.PrimeNormNum

Conversation

@fpvandoorn
Copy link
Copy Markdown
Member

@fpvandoorn fpvandoorn commented May 10, 2023

  • Excluding computing Nat.factors, which requires a little bit of generalization of NormNum.Result.
  • It's a lot faster than in Lean 3:
2 ^ 19 - 1 is prime: Lean 3: 330ms, Lean 4: 36ms
2 ^ 25 - 39 is prime: Lean 3: 3300ms + multiple seconds type-checking, Lean 4 165ms + <100ms type-checking
(2 ^ 19 - 1) * (2 ^ 25 - 39) is not prime: Lean 3: 300ms, Lean 4: 90ms
  • Simplify the definition of Nat.minFac a bit
  • Added some remarks in the code, explaining why some parts are a bit more verbose than necessary. In these cases, simplifying the code would cause a significant slow-down (my first version of this port was 100x slower).
  • Note: Calling the function on larger numbers currently does cause panics/stack overflows in Lean 4

Open in Gitpod

@fpvandoorn fpvandoorn added the WIP Work in progress label May 10, 2023
@fpvandoorn fpvandoorn added awaiting-review awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. and removed WIP Work in progress labels May 13, 2023
@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 May 13, 2023
@fpvandoorn fpvandoorn added the t-meta Tactics, attributes or user commands label May 13, 2023
@fpvandoorn
Copy link
Copy Markdown
Member Author

fpvandoorn commented May 15, 2023

Note: conflicts with #3923

I'm planning to move the file to the Tactic/NormNum folder and merge the test file with that of #3923, assuming we merge that one first.

@fpvandoorn fpvandoorn requested review from digama0 and kmill May 15, 2023 10:01
@kim-em kim-em added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label May 16, 2023
@kim-em kim-em removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label May 16, 2023
@digama0
Copy link
Copy Markdown
Member

digama0 commented May 16, 2023

Rather than using let binders (which I'm not sure work anyway for this kind of thing), I would simply suggest using tree-like proofs instead of list-like: it's a search, so we can easily use branching proofs saying that if you are good from k=a to k=b and also from k=b to k=c then you are good from k=a to k=c, and apply that instead of just adding 2 and using a subterm for the rest.

@kim-em kim-em added awaiting-author A reviewer has asked the author a question or requested changes. and removed awaiting-review labels May 16, 2023
@fpvandoorn fpvandoorn added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes. labels May 17, 2023
@fpvandoorn
Copy link
Copy Markdown
Member Author

Can we do this in a follow-up PR? The current function works well for primes up to 25 bits. For much larger primes we'll want a more efficient implementation anyway.


theorem minFacHelper_0 (n : ℕ) (h1 : ¬ n = 1) (h2 : ¬ n % 2 = 0) : MinFacHelper n 3 := by
theorem minFacHelper_0 (n : ℕ)
(h1 : Nat.ble (nat_lit 2) n = true) (h2 : nat_lit 1 = n % (nat_lit 2)) :
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.

The h2 equality is flipped here because the elaborator doesn't seem to like it the other way around, when you copy and paste a norm_num proof directly (useful for debugging). We might consider doing this more broadly...

theorem isNat_prime_2 : {n n' : ℕ} → IsNat n n' → Nat.ble 2 n' = true → minFac n = n → n.Prime
| _, _, ⟨rfl⟩, h1, h2 => prime_def_minFac.mpr ⟨ble_eq.mp h1, h2⟩

theorem isNat_not_prime {n n' : ℕ} (h : IsNat n n') : ¬n'.Prime → ¬n.Prime := isNat.natElim h
Copy link
Copy Markdown
Member

@digama0 digama0 May 17, 2023

Choose a reason for hiding this comment

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

This is to avoid bound variables / lambda abstraction in norm_num proofs

@fpvandoorn
Copy link
Copy Markdown
Member Author

I take it you don't really like the deriveBool operation? It's definitely a bit slower than avoiding it, but I thought it was nice to get a bool-result with the correct type (up to defeq). Your changes LGTM.

@fpvandoorn
Copy link
Copy Markdown
Member Author

@digama0 Shall we merge this?

@digama0
Copy link
Copy Markdown
Member

digama0 commented May 22, 2023

I wasn't sure whether we should merge #4048 before or after this one since otherwise there is this bad interaction with minFac 1000 + 0 as you observed, but I guess they are mostly independent.

bors r+

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels May 22, 2023
bors bot pushed a commit that referenced this pull request May 22, 2023
* Excluding computing `Nat.factors`, which requires a little bit of generalization of `NormNum.Result`.
* It's a lot faster than in Lean 3:
```
2 ^ 19 - 1 is prime: Lean 3: 330ms, Lean 4: 36ms
2 ^ 25 - 39 is prime: Lean 3: 3300ms + multiple seconds type-checking, Lean 4 165ms + <100ms type-checking
(2 ^ 19 - 1) * (2 ^ 25 - 39) is not prime: Lean 3: 300ms, Lean 4: 90ms
```
* Simplify the definition of `Nat.minFac` a bit
* Added some remarks in the code, explaining why some parts are a bit more verbose than necessary. In these cases, simplifying the code would cause a significant slow-down (my first version of this port was 100x slower).
* Note: Calling the function on larger numbers currently does cause panics/stack overflows in Lean 4



Co-authored-by: Mario Carneiro <di.gama@gmail.com>
@bors
Copy link
Copy Markdown

bors bot commented May 23, 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 feat: port Data.Nat.PrimeNormNum [Merged by Bors] - feat: port Data.Nat.PrimeNormNum May 23, 2023
@bors bors bot closed this May 23, 2023
@bors bors bot deleted the port/Data.Nat.PrimeNormNum branch May 23, 2023 00:09
Parcly-Taxel added a commit that referenced this pull request May 23, 2023
bors bot pushed a commit that referenced this pull request May 23, 2023
The `norm_num` extension was already ported in #3892, so this PR adopts the same approach as #3816.
mathlib-bors bot pushed a commit that referenced this pull request Feb 6, 2024
* Uses `deriveBool`, which was added in #3892, and allows removal of some `have` lines.
* Changes an `if then else` into a match. This is more readable in my opinion.
atarnoam pushed a commit that referenced this pull request Feb 9, 2024
* Uses `deriveBool`, which was added in #3892, and allows removal of some `have` lines.
* Changes an `if then else` into a match. This is more readable in my opinion.
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-meta Tactics, attributes or user commands

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants