problem in evaluation dual isogeny

[chriswuthrich]

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'>

Component: elliptic curves

Keywords: isogenies

Author: Chris Wuthrich

Reviewer: Luca De Feo, John Cremona

Merged: sage-4.7.1.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/10888

[chriswuthrich]

comment:1

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'>

[JohnCremona]

comment:2

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.

[JohnCremona]

comment:3

Replying to @JohnCremona:

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.

[chriswuthrich]

comment:4

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.

[chriswuthrich]

only uploaded for discussion

[chriswuthrich]

Attachment: trac_10888_evaluation_of_multi_polynomials.patch.gz

Attachment: trac_10888.patch.gz

exported against 4.6.2. Only this patch should be applied.

[chriswuthrich]

Author: wuthrich

[chriswuthrich]

comment:5

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.

[chriswuthrich]

comment:6

The other, more general issue is now #10946.

[defeo]

comment:7

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.

[JohnCremona]

comment:8

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.

[JohnCremona]

Reviewer: Luca De Feo, John Cremona

[jdemeyer]

Changed author from wuthrich to Chris Wuthrich

[jdemeyer]

Merged: sage-4.7.1.alpha0