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] - feat: port field_simp tactic #1824

Closed
wants to merge 9 commits into from
Closed

Conversation

dwrensha
Copy link
Member

@dwrensha dwrensha commented Jan 24, 2023

@dwrensha dwrensha added awaiting-review t-meta Tactics, attributes or user commands labels Jan 24, 2023
| .ok (some _) => return m!"{checkEmoji} discharge {prop}"

/-- Discharge strategy for the field_simp tactic. -/
partial def discharge (prop : Expr) : SimpM (Option Expr) :=
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@fpvandoorn fpvandoorn left a comment

Choose a reason for hiding this comment

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

Since there seem to be quite some differences between the mathlib3 and mathlib4 implementations of field_simps, it might be good to test this on more examples, if it is reasonably easy to add more examples...

I saw three commented out cases in test/ring. If I add

import Mathlib.Tactic.FieldSimp
import Mathlib.Data.Real.Basic

the first two work, but are incredibly slow, and I think the third one is supposed to further simplify 2 * x ^ 3 * 2 / (24 * x) and fails.

let r ← elabSimpArgs (sa.getD ⟨.missing⟩) ctx (eraseLocal := false) .simp

let _ ← simpLocation r.ctx dis loc
pure ()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pure ()

@fpvandoorn fpvandoorn added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review labels Jan 27, 2023
@dwrensha
Copy link
Member Author

Re-added a call to NormNum.derive in discharge. That brings the implementation more in line with mathlib3, and gets the previously-broken real-numbers example in test/ring.lean to work.
(Note that #1711 the real numbers get mocked out in that test file. My fix works on both the real real numbers and on the mocked-out one.)


let prop : Q(Prop) ← (do pure prop)
let pf? ← match prop with
| ~q(($e : $α) ≠ $b) =>
Copy link
Member Author

@dwrensha dwrensha Jan 27, 2023

Choose a reason for hiding this comment

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

The mathlib3 version instead matches on $e ≠ 0. I couldn't figure out a way to make that work with Qq. (I could get it to typecheck here, but then it failed to actually match the expressions that we need it to.) This more general match should be fine. The cost is that we might call NormNum.derive slightly more often than is strictly necessary.

@dwrensha
Copy link
Member Author

I'm guessing that the slowness of the first two examples in test/ring.lean has to do with leanprover/lean4#2055.

@dwrensha dwrensha added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jan 27, 2023
@dwrensha
Copy link
Member Author

After #2188, the examples in test/ring seem to be going a lot faster.
The commented-out example in test/FieldSimp no longer needs a maxHeartbeats adjustment to succeed, but it's still slow enough that I think it makes sense to keep it commented out.

@fpvandoorn
Copy link
Member

Ok, let's get this merged

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Feb 10, 2023
bors bot pushed a commit that referenced this pull request Feb 10, 2023
* Ports the `field_simp` tactic and tests from mathlib3.
* Adds a new test taken from minif2f (but with rats instead of reals, for simplicity): https://github.com/openai/miniF2F/blob/4e433ff5cadff23f9911a2bb5bbab2d351ce5554/lean/src/valid.lean#L524-L530.
* Uses `field_simp` in two already-ported files, as directed by port notes.
* Adds an explicit priority to a few `field_simps` lemmas. This is necessary to make all of the examples work.
@bors
Copy link

bors bot commented Feb 10, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: port field_simp tactic [Merged by Bors] - feat: port field_simp tactic Feb 10, 2023
@bors bors bot closed this Feb 10, 2023
@bors bors bot deleted the field_simp branch February 10, 2023 22:33
urkud pushed a commit that referenced this pull request Feb 11, 2023
* Ports the `field_simp` tactic and tests from mathlib3.
* Adds a new test taken from minif2f (but with rats instead of reals, for simplicity): https://github.com/openai/miniF2F/blob/4e433ff5cadff23f9911a2bb5bbab2d351ce5554/lean/src/valid.lean#L524-L530.
* Uses `field_simp` in two already-ported files, as directed by port notes.
* Adds an explicit priority to a few `field_simps` lemmas. This is necessary to make all of the examples work.
mo271 pushed a commit that referenced this pull request Feb 18, 2023
* Ports the `field_simp` tactic and tests from mathlib3.
* Adds a new test taken from minif2f (but with rats instead of reals, for simplicity): https://github.com/openai/miniF2F/blob/4e433ff5cadff23f9911a2bb5bbab2d351ce5554/lean/src/valid.lean#L524-L530.
* Uses `field_simp` in two already-ported files, as directed by port notes.
* Adds an explicit priority to a few `field_simps` lemmas. This is necessary to make all of the examples work.
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.

None yet

2 participants