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

wrap nsatz with dropRingSyntax; port CompleteEdwardsCurveTheorems to super_nsatz #16

Closed
wants to merge 8 commits into from

Conversation

andres-erbsen
Copy link
Contributor

See #15.

bash_step.
bash_step.
bash_step.
{ conservative_common_denominator.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasonGross : Is conservative_common_denominator intended to be idempotent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. It should leave the primary goal with no divisions in it, and should fail when there is no division. Does it currently do wacky things with the side-conditions? If so, we should guard it with a check that the goal is a field equality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, oops, it doesn't work with fractions in denominators yet. I'll fix that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be fixed by b707a30 (haven't tested it on this example, though)

JasonGross added a commit that referenced this pull request Jun 29, 2016
JasonGross added a commit that referenced this pull request Jun 29, 2016
{ conservative_common_denominator_all. (* FIXME: doesn't do anything *)
common_denominator_all. nsatz. auto using a_d_y2_nonzero. }
unfold solve_for_x2; simpl; split; intros;
(conservative_common_denominator_all; [nsatz | eapply a_d_y2_nonzero; eauto]).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

conservative_common_denominator_all generates a goal False here, probably by doing something equivalent to unfold not; intro. It would be nice if it left goals of the form _ <> 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed by 9f3c224

JasonGross added a commit that referenced this pull request Jun 29, 2016
We no longer try to predict field_simplify_eq.  This results in better
behavior and less code which is more modular.  In particular, the tactic
responsible for hiding non-fraction pieces from field_simplify_eq no
longer tries to preemptively assert that denominators are nonzero.

This improvement is a result of @andres-erbsen's point in #16,
#16 (comment) ,
that we were generating too many side-conditions.
@@ -633,6 +633,16 @@ Module Field.
End Homomorphism.
End Field.

(*** Tactics *)

Module Import Algebra_syntax_Nsatz.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason to do it like this? If you replace

Tactic Notation "nsatz" := nsatz 1%nat || nsatz 2%nat || nsatz 3%nat || nsatz 4%nat || nsatz 5%nat.

with

Ltac nsatz := nsatz 1%nat || nsatz 2%nat || nsatz 3%nat || nsatz 4%nat || nsatz 5%nat.
Tactic Notation "nsatz" := nsatz.

(assuming I haven't messed up my Ltac parsing; if I have, you'll need to do

Ltac nsatz_power n :=
  let nn := (eval compute in (BinNat.N.of_nat n)) in
  nsatz_sugar_power BinInt.Z0 nn.
Ltac nsatz := nsatz_power 1%nat || nsatz_power 2%nat || nsatz_power 3%nat || nsatz_power 4%nat || nsatz_power 5%nat.
Tactic Notation "nsatz" := nsatz.
Tactic Notation "nsatz" constr(n) := nsatz_power n.

), then you can just do Ltac nsatz := Algebra_syntax.Nsatz.nsatz; dropRingSyntax, which I think is slightly nicer.

@JasonGross
Copy link
Collaborator

What's the status of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants