Opened 9 years ago

Closed 8 years ago

#10888 closed defect (fixed)

problem in evaluation dual isogeny

Reported by: wuthrich Owned by: cremona
Priority: major Milestone: sage-4.7.1
Component: elliptic curves Keywords: isogenies,
Cc: Merged in: sage-4.7.1.alpha0
Authors: Chris Wuthrich Reviewers: Luca De Feo, John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The following bug was reported by J. Gillibert.

sage: K.<th> = NumberField(x^2+3)
sage: E = EllipticCurve(K,[7,0])
sage: phi = E.isogeny(E(0,0))
sage: P = E(-3,4*th)
sage: phi(P)
(-16/3 : 8/9*th : 1)
sage: Q = phi(P)
sage: phihat = phi.dual()
sage: phihat(Q)

This gives back a

TypeError: <type 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular'>

Attachments (2)

trac_10888_evaluation_of_multi_polynomials.patch (1.7 KB) - added by wuthrich 9 years ago.
only uploaded for discussion
trac_10888.patch (2.4 KB) - added by wuthrich 9 years ago.
exported against 4.6.2. Only this patch should be applied.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by wuthrich

The problem is very simply that evaluation in a polynomial with two variables does not give back a constant, but a constant polynomial. Is this a bug ?

sage: R.<x,y> = PolynomialRing(ZZ,2)
sage: f(x=2,y=0)
-2
sage: type(f(x=2,y=0))
<type 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular'>

It is certainly not consitent with this :

sage: R.<x> = PolynomialRing(ZZ)
sage: f= x^2+3
sage: type(f(x=2))
<type 'sage.rings.integer.Integer'>

comment:2 follow-up: Changed 9 years ago by cremona

I came across something very similar indeed before and thought I had fixed it. I should check to see if the patch I made ever got merged.

comment:3 in reply to: ↑ 2 Changed 9 years ago by cremona

Replying to cremona:

I came across something very similar indeed before and thought I had fixed it. I should check to see if the patch I made ever got merged.

See #8502.

comment:4 Changed 9 years ago by wuthrich

John, thanks to your link I found quickly what is the problem. Consider

f(2,3)
f(x=2,y=3)
f.subs(x=2,y=3)

The first returns an element in the base ring, while the others are still polynomials. This is correct with respect to the documentation apart from the middle one. f(x=2,y=3) is the __call__ method and there, in the first lines, it jumps to execute subs.

So something has to be changed there. I am uncertain what. I attach a patch that changes __call__ to make it coherent with the documentation, but it is not ready for review because it will most certainly produce many problems in other places...

I will aks this to sage-devel.

Changed 9 years ago by wuthrich

only uploaded for discussion

Changed 9 years ago by wuthrich

exported against 4.6.2. Only this patch should be applied.

comment:5 Changed 9 years ago by wuthrich

  • Authors set to wuthrich
  • Milestone set to sage-4.7
  • Status changed from new to needs_review

From the discussion on sage-devel, I learned that I should not change the functioning of evaluation in this ticket. To solve the bug in the isogenies, I simply make the evaluations in the code to be evaluations without keywords. Then it is no longer ambiguous.

I will open another ticket about the issue with evaluation and substitution.

comment:6 Changed 9 years ago by wuthrich

The other, more general issue is now #10946.

comment:7 Changed 8 years ago by defeo

  • Cc defeo added
  • Status changed from needs_review to needs_work

The patch works and I'm willing to give a positive review, but the doctest takes 2.6 cpu secs on my laptop (64-bit Intel Core2 U9400 @ 1.40GHz... not the fastest cpu ever, but still decent), it would make sense to tag this test as long.

Alternatively, though I don't know if this is acceptable for Sage standards, the doctest could be slightly changed. The code below fails in 4.6.2 and is fixed by this patch; it takes only 0.2 secs, because it uses Kohel's algorithm (instead of Vélu's formulae) to compute phi and because avoids the call to phi.dual() (which uses Stark's algorithm). It tests exactly the same bug, because it crosses the same conditional branches modified by the patch.

sage: K.<th> = NumberField(x^2+3)
sage: E = EllipticCurve(K,[7,0])
sage: _.<X> = K[]
sage: phi = E.isogeny(X)
sage: phi.set_pre_isomorphism(E.automorphisms()[1])
sage: phi.set_post_isomorphism(phi.codomain().automorphisms()[1])
sage: P = E(-3,4*th)
sage: phi(P)
(-16/3 : 8/9*th : 1)

Finally, the doctest does not show up in the reference manual, because it is in the docstring of the method __call__. I don't know if this really is a problem.

comment:8 Changed 8 years ago by cremona

  • Cc defeo removed
  • Reviewers set to Luca De Feo, John Cremona
  • Status changed from needs_work to positive_review

I have also tested the patch and agree that it works (applied to 4.7.alpha5, all tests in elliptic curves pass).

I don't think that the test is too long. This whole file is quite long as a whole (nearly 30s) but several other tests are longer! I expect to be splitting this file into two before long, so I am happy to leave off the "long time" tag. I would also be happy to have defeo's additional code added.

It doesn't matter that this example does not appear in the reference manual. It stays in as a test, to make sure this will continue to work in future; but it does not add a lot to for the user, I think. If you want it to appear in the manual, insert it in the header section of the file (instead, or as well).

I'll set this to "positive review" now; if you (defeo) wants to make further changes to the tests put it back to "needs review" and I'll look at it again.

comment:9 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1

comment:10 Changed 8 years ago by jdemeyer

  • Authors changed from wuthrich to Chris Wuthrich

comment:11 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.