-
Notifications
You must be signed in to change notification settings - Fork 21
Add new tactic "ensatz" for proving polynomial equalities with existential quantifier #160
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
base: master
Are you sure you want to change the base?
Conversation
This is an exciting feature. Would it be feasible to add this without another copy of the nsatz Ltac and especially the reification procedure? (Is this tactic intended to also prove all goals the previous one proves?) |
Reading https://www.cl.cam.ac.uk/~jrh13/papers/divisibility.pdf theorem 5 i understand how to solve goals where Y_1..Y_m appear as linear-combination coefficients. Is the algorithm in this PR more general, or should Q(...) above be restricted more? |
The |
You are right, Q(...) is restricted to the cases where Y_1 ... Y_m appear as linear-combination coefficients. |
theories/nsatz/ENsatzTactic.v
Outdated
|
||
Examples: see test-suite/success/ENsatz.v | ||
|
||
Reification is done using type classes, defined in Ncring_tac.v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the entry point for reification is a typeclass Hint Extern, but the reification is ltac not typeclass based AFAIK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the line which was a left over from copying/pasting the NsatzTactic.v file.
05dcc41
to
fa8e0d2
Compare
I think there's a good chance a bunch of code could be shared. Whether it's worth it or not is for us to determine. But for example the hol-light implementation does seem to share code (examples). |
fa8e0d2
to
034067d
Compare
9474023
to
f48b1bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be good having a tactic like this in stdlib. And I'd like to use it myself, both in stdlib and otherwise. At the same time, the proposed implementation provoked a number of initial responses and questions from me. This initial review is not complete and not everything here is requirement for merging, but I would like to hear your thoughts on these points and overall plan/goals.
Additionally, are you looking to keep using this tactic yourself, and where?
Edit: an important part of my context here is that I am a heavy user of nsatz and have spent a substantial amount of time fighting its implementation issues, including maintaining a part-reimplementation (in fiat-crypto). From this, my overall impression is that nsatz .v-file code is not something to be imitated, and every bit of this kind of in the repo is a liability (it is obviously also an asset). And not-thought-through implementation behaviors do tend to acquire rigid users, making them much harder to change later. rocq-prover/rocq#18325 is an example, #155 (comment) (WIP) is another.
+ Add support for existential quantifier in nsatz | ||
(`#160 <https://github.com/coq/stdlib/pull/160>`_, | ||
by Lionel Blatter). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check add the documentation and link to it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the remark. Is the changelog entry incomplete in terms of references to the new tactic's documentation? If so, I don't know how to proceed, since the documentation page for nsatz is in the Rocq project and not Stdlib. I wrote documentation for ensatz in my fork of Rocq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies for the broken sentence. Yes, I meant that it would be nice to have a documentation link in the changelog (and have it committed, somewhere). Example.
Where to actually put the documentation is indeed an interesting question. There are actually three plausible places: rocq refman, stdlib refman, and stdlib html+coqdoc documentation. And I'm not sure what the high-level plan is for documentation or ml-supported tactics in the rocq-stdlib split. But documenting next to nsatz (and making a linked PR) seems fine.
unfold divides, modulo, coprime, ideal, gcd. | ||
preprocess. | ||
Time ensatz. | ||
Qed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More tests would significantly increase my expectations of success for trying to use this tactic. E.g. the hol-light examples that are proven by INTEGER_TAC or INTEGER_RULE should be appropriate. If some of them don't work for a well-understood reason, documenting that is especially important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a first batch of additional examples. Maybe I'll add a few more.
theories/nsatz/ENsatzTactic.v
Outdated
Definition merge {A R:Type} (p1 p2: A -> @MPoly R) := | ||
fun x => add_mpoly (p1 x) (opp_mpoly (p2 x)). | ||
|
||
Ltac reify_goal := |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This procedure interleaves generalization of the goal over effective variables (from the perspective of ensatz) and the reification of it. For troubleshooting failing invocations, it is useful to be able to run the generalization phase without reification. (This also enables faster and more predictable reification methods.) Existing stdlib tactics do do something similar to the code here, and I am not going to hold this contribution to a hypocritically high standard, but I do genuinely believe it would be more useful if these steps were factored out.
theories/nsatz/ENsatzTactic.v
Outdated
let lv := fresh "lv" in | ||
set (p21:=p2); set (lp21:=lp2); set (n21:= n); set(lv := fv); | ||
(*We want r to be 1, so we set radicalmax to 1*) | ||
NsatzTactic.nsatz_call 1%N info 0%Z p2 lp2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have two calls to this function, it would be really nice to have at least a basic comment describing what it should and should not do.
theories/nsatz/ENsatzTactic.v
Outdated
let p211 := constr: (PEmul c p21) in | ||
(*If c is not 1/-1, this assert should fail*) | ||
assert (Hg:check lp21 p211 (lci,lq) = true); | ||
[vm_compute; reflexivity |]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this vm_compute always succeed if there are no bugs in this tactic? If yes, consider vm_cast_no_check (eq_refl true)
theories/nsatz/ENsatzTactic.v
Outdated
ltac:(fun c r lq lci => | ||
let Hg := fresh "Hg" in | ||
let c21 := fresh "c" in | ||
set (c21 := c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intent to perform unification of all subterms of the entire goal with c here? If not, pose and change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is intentional.
@@ -0,0 +1,6 @@ | |||
- `ENsatzTactic.v` and `ENsatz.v` | |||
|
|||
+ Add support for existential quantifier in nsatz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the plan? Currently, the PR adds new tactic ensatz
, without adding support for existential quantifiers to nsatz
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I change the entry message.
f48b1bc
to
3f6873b
Compare
My original plan was to use this tactic in Easycrypt. It is not certain that this will happen in the end. I will apply patches to answer the question as best I can. The ongoing re-implementation of nsatz is in conflict with ensatz and will require more time to patch. |
Just to be clear, I am not asking you to step in to resolve the situation with nsatz being duplicated in stdlib and fiat-crypto. And my plan there is to backport fixes from fiat-crypto in small minimally invasive changes, hopefully reaching a state where stdlib nsatz fits both use cases. The reason I am pointing out this duplication is to advocate for avoiding further duplication within stdlib: code shared between ensatz and nsatz will benefit from these backports, whereas dedicated ensatz code will not. |
e1566d5
to
5b33f05
Compare
I created a file |
e68cec1
to
49de007
Compare
Indeed, merging seems preferable to duplicating opaque-yet-relevant instances. Though, maybe ensatz just works (for Z) when ZNsatz is imported. |
After reading the documentation, there's one more thing on my mind: the |
I wouldn't say so (other than refining the goal evar as all tactics do). For instance what evars does esplit in your example instantiate? |
4f090f3
to
314d075
Compare
314d075
to
bbcc685
Compare
I have the impression that even if the |
Agreed. Overall, my intuition is that merging ensatz and nsatz would be great, both for user experience and (I hope) maintenance. Do you think it would be practical? As for support for |
On a third thought, isn't the difference between |
Apparently, the I will give a try to add support for instantiating existential variables. |
bbcc685
to
8dab64e
Compare
…ntial quantifier Co-authored-by: Laurent Thery <[email protected]>
8dab64e
to
8363867
Compare
I finally found the time to add the support for existential variables. I still have to refactor the code a bit and complete the documentation. The source of the error in the CI is not clear to me. |
8363867
to
f51dbb6
Compare
This PR introduce the
ensatz
tactic that can prove automatically goals of the form:Tests have be added to the test-suite in file
test-suite/success/ENsatz.v
.An entry to the sphinx documentation of
Rocq
is available here and ready for a PR.