some elliptic curve functions over number fields fail over relative fields

[JohnCremona]

This was reported by Alejandro Argaez:

sage: K1.<w>=NumberField(x^2+x+1)             
sage: m=polygen(K1)
sage: K2.<v>=K1.extension(m^2-w+1)
sage: E=EllipticCurve([0*v,-432])
sage: E.global_minimal_model()
<boom>

The error is that the degree() function is called on the ring of integers of a relative number field.

In fixing this bug (which should be easy) it would be a good idea to add relative examples to as many functions as possible in ell_numberfield.py

apply attachment: trac_14472-elliptic_curves_jd.patch

CC: alejandroargaez@hotmail.com

Component: elliptic curves

Keywords: elliptic curve relative number field

Author: John Cremona, Jeroen Demeyer

Reviewer: Jeroen Demeyer, John Cremona

Merged: sage-5.10.beta1

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

[JohnCremona]

comment:1

Note: after changing ZK.degree() to ZK.absolute_degree() on line 651 of ell_numberfield.py it still fails, since the code in lines 656--658 (which I wrote, I think) does not work for relative number fields. The purpose of these lines is to set r,s,t to "least residues" modulo 2,3,2 of three successive quantities.

[JohnCremona]

comment:2

Replying to @JohnCremona:

Note: after changing ZK.degree() to ZK.absolute_degree() on line 651 of ell_numberfield.py it still fails, since the code in lines 656--658 (which I wrote, I think) does not work for relative number fields. The purpose of these lines is to set r,s,t to "least residues" modulo 2,3,2 of three successive quantities.

Follow-up: the old code for _reduce_model() was flawed, as follows: to reduce a1,a2,a3 modulo 2,3,2 respectively, it attempted to reduce their coordinates as given by list(a1), etc. Firstly this fails for relative extensions, but it is also misguided since there is no reason why the list() coordinates should be integral. I have changed this to work with the coordinates with respect to an integral basis, which is good for relative extensions. Only one small problem: the doctest on line 860 which used to return (as a minimal model over Q(a) where a=sqrt(5))

(0, 1, 0, a - 33, -2*a + 64)

but now gives

(0, -3/2*a - 1/2, 0, 3/2*a - 59/2, 27/2*a + 155/2)

which does not look so nice. Note that the integral basis here is [1/2a + 1/2, a] and that with respect to this basis 1 has coordinates (2,-1) while -3/2a - 1/2 has coordinates (-1,-1) so (counterintuitively) 1 is not reduced mod 2 but -(3a+1)/2 is!

Note that the integral basis computed does depend on the polynomial used to generate the field:

sage: QuadraticField(5,'a').ring_of_integers().gens()                                                   
[1/2*a + 1/2, a]
sage: NumberField(x^2-x-1,'a').ring_of_integers().gens()                                                
[1, a]

[JohnCremona]

Applies to 5.9.beta5

[JohnCremona]

comment:3

Attachment: trac_14472-elliptic_curves.patch.gz

[JohnCremona]

Author: John Cremona

[jdemeyer]

comment:4

All the examples seem to be quadratic number fields, is this intentional?

I don't know why Sage returns the basis of ZK like that, because it's not what PARI gives:

sage: K.<a> = NumberField(x^2-5)
sage: K.integral_basis()
[1/2*a + 1/2, a]
sage: K._pari_integral_basis()
[1, 1/2*y - 1/2]

As for reducing an element modulo an ideal (which is what you do here), you could use PARI's nfeltreduce().

[JohnCremona]

comment:5

Replying to @jdemeyer:

All the examples seem to be quadratic number fields, is this intentional?

No, probably just laziness.

I don't know why Sage returns the basis of ZK like that, because it's not what PARI gives:

sage: K.<a> = NumberField(x^2-5)
sage: K.integral_basis()
[1/2*a + 1/2, a]
sage: K._pari_integral_basis()
[1, 1/2*y - 1/2]

Well spotted. The integral_basis method calls maximal_order which does call _pari_integral_basis, but then applies some Order constructor to the generators (order.absolute_order_from_module_generators) which is where this non-canonical ( to my mind) basis comes from. If that is to be chaned for quadratic fields then that would be a separate ticket, and would surely have a lot of doctest output consequences.

As for reducing an element modulo an ideal (which is what you do here), you could use PARI's nfeltreduce().

Sure, but here we are only reducing modulo (2) or (3) so it seemed easier to do it manually.

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:6

I made a new patch using PARI's nfeltdiveuc. This gives simpler code and has the advantage that 1 is reduced, so there is no need to change the field.

[JohnCremona]

comment:8

Replying to @jdemeyer:
I like the use of nfeltdiveuc, but (as you have maybe already noticed) the values of r,s,t are not the reduced values....so I'll wait for the next version of your patch!

[jdemeyer]

comment:9

Replying to @JohnCremona:

Replying to @jdemeyer:
I like the use of nfeltdiveuc, but (as you have maybe already noticed) the values of r,s,t are not the reduced values...

Sorry, I don't understand what you mean. Can you say more precisely what is wrong?

[JohnCremona]

comment:10

Sorry, I think I misunderstood the output of nfeltdiveuc. I see now that it gives the quotient, not the remainder, and that is correct. So it is good. I am testing -- so why does it need work?

Also, I am getting an warning that line 9821 in sage.libs.pari.gen.pyx is unreachable code. Does tat need looking into?

[jdemeyer]

comment:12

Attachment: trac_14472-elliptic_curves_jd.patch.gz

Replying to @JohnCremona:

Also, I am getting an warning that line 9821 in sage.libs.pari.gen.pyx is unreachable code. Does tat need looking into?

That has nothing to do with this ticket, but I fixed it anyway.

[jdemeyer]

comment:13

John, do you want to formally review my patch? I give positive review to the parts which were copied from your initial patch.

[JohnCremona]

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, John Cremona

[JohnCremona]

Changed author from John Cremona to John Cremona, Jeroen Demeyer

[JohnCremona]

comment:14

I give positive review to the parts which Jeroen added, hance we have an overall positive review.

[jdemeyer]

Merged: sage-5.10.beta1