Opened 7 years ago

Closed 7 years ago

#14472 closed defect (fixed)

some elliptic curve functions over number fields fail over relative fields

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-5.10
Component: elliptic curves Keywords: elliptic curve relative number field
Cc: alejandroargaez@… Merged in: sage-5.10.beta1
Authors: John Cremona, Jeroen Demeyer Reviewers: Jeroen Demeyer, John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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 trac_14472-elliptic_curves_jd.patch

Attachments (2)

trac_14472-elliptic_curves.patch (3.6 KB) - added by cremona 7 years ago.
Applies to 5.9.beta5
trac_14472-elliptic_curves_jd.patch (5.2 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 7 years ago by cremona

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.

comment:2 in reply to: ↑ 1 Changed 7 years ago by cremona

Replying to cremona:

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/2*a + 1/2, a] and that with respect to this basis 1 has coordinates (2,-1) while -3/2*a - 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]

Changed 7 years ago by cremona

Applies to 5.9.beta5

comment:3 Changed 7 years ago by cremona

  • Authors set to John Cremona
  • Status changed from new to needs_review

comment:4 follow-up: Changed 7 years ago by jdemeyer

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().

comment:5 in reply to: ↑ 4 Changed 7 years ago by cremona

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.

comment:6 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer

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.

comment:7 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by cremona

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!

comment:9 in reply to: ↑ 8 Changed 7 years ago by jdemeyer

Replying to cremona:

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?

comment:10 follow-up: Changed 7 years ago by cremona

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?

comment:11 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Changed 7 years ago by jdemeyer

comment:12 in reply to: ↑ 10 Changed 7 years ago by jdemeyer

Replying to cremona:

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.

comment:13 Changed 7 years ago by jdemeyer

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

comment:14 Changed 7 years ago by cremona

  • Authors changed from John Cremona to John Cremona, Jeroen Demeyer
  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, John Cremona
  • Status changed from needs_review to positive_review

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

comment:15 Changed 7 years ago by jdemeyer

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