Sage: Ticket #14472: some elliptic curve functions over number fields fail over relative fields
https://trac.sagemath.org/ticket/14472
<p>
This was reported by Alejandro Argaez:
</p>
<pre class="wiki">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>
</pre><p>
The error is that the degree() function is called on the ring of integers of a relative number field.
</p>
<p>
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 <code>ell_numberfield.py</code>
</p>
<p>
<strong>apply</strong> <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/14472/trac_14472-elliptic_curves_jd.patch" title="Attachment 'trac_14472-elliptic_curves_jd.patch' in Ticket #14472">trac_14472-elliptic_curves_jd.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/14472/trac_14472-elliptic_curves_jd.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/14472
Trac 1.1.6cremonaSun, 21 Apr 2013 16:38:00 GMT
https://trac.sagemath.org/ticket/14472#comment:1
https://trac.sagemath.org/ticket/14472#comment:1
<p>
Note: after changing ZK.degree() to ZK.absolute_degree() on line 651 of <code>ell_numberfield.py</code> 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.
</p>
TicketcremonaMon, 22 Apr 2013 09:14:47 GMT
https://trac.sagemath.org/ticket/14472#comment:2
https://trac.sagemath.org/ticket/14472#comment:2
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14472#comment:1" title="Comment 1">cremona</a>:
</p>
<blockquote class="citation">
<p>
Note: after changing ZK.degree() to ZK.absolute_degree() on line 651 of <code>ell_numberfield.py</code> 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.
</p>
</blockquote>
<p>
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))
</p>
<pre class="wiki">(0, 1, 0, a - 33, -2*a + 64)
</pre><p>
but now gives
</p>
<pre class="wiki">(0, -3/2*a - 1/2, 0, 3/2*a - 59/2, 27/2*a + 155/2)
</pre><p>
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!
</p>
<p>
Note that the integral basis computed does depend on the polynomial used to generate the field:
</p>
<pre class="wiki">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]
</pre>
TicketcremonaMon, 22 Apr 2013 10:19:44 GMTattachment set
https://trac.sagemath.org/ticket/14472
https://trac.sagemath.org/ticket/14472
<ul>
<li><strong>attachment</strong>
set to <em>trac_14472-elliptic_curves.patch</em>
</li>
</ul>
<p>
Applies to 5.9.beta5
</p>
TicketcremonaMon, 22 Apr 2013 10:20:11 GMTstatus changed; author set
https://trac.sagemath.org/ticket/14472#comment:3
https://trac.sagemath.org/ticket/14472#comment:3
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>John Cremona</em>
</li>
</ul>
TicketjdemeyerTue, 23 Apr 2013 07:20:34 GMT
https://trac.sagemath.org/ticket/14472#comment:4
https://trac.sagemath.org/ticket/14472#comment:4
<p>
All the examples seem to be <em>quadratic</em> number fields, is this intentional?
</p>
<p>
I don't know why Sage returns the basis of <code>ZK</code> like that, because it's not what PARI gives:
</p>
<pre class="wiki">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]
</pre><p>
As for reducing an element modulo an ideal (which is what you do here), you could use PARI's <code>nfeltreduce()</code>.
</p>
TicketcremonaTue, 23 Apr 2013 08:28:06 GMT
https://trac.sagemath.org/ticket/14472#comment:5
https://trac.sagemath.org/ticket/14472#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14472#comment:4" title="Comment 4">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
All the examples seem to be <em>quadratic</em> number fields, is this intentional?
</p>
</blockquote>
<p>
No, probably just laziness.
</p>
<blockquote class="citation">
<p>
I don't know why Sage returns the basis of <code>ZK</code> like that, because it's not what PARI gives:
</p>
<pre class="wiki">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]
</pre></blockquote>
<p>
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.
</p>
<blockquote class="citation">
<p>
As for reducing an element modulo an ideal (which is what you do here), you could use PARI's <code>nfeltreduce()</code>.
</p>
</blockquote>
<p>
Sure, but here we are only reducing modulo (2) or (3) so it seemed easier to do it manually.
</p>
TicketjdemeyerTue, 23 Apr 2013 09:32:51 GMTdescription changed; reviewer set
https://trac.sagemath.org/ticket/14472#comment:6
https://trac.sagemath.org/ticket/14472#comment:6
<ul>
<li><strong>reviewer</strong>
set to <em>Jeroen Demeyer</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/14472?action=diff&version=6">diff</a>)
</li>
</ul>
<p>
I made a new patch using PARI's <code>nfeltdiveuc</code>. This gives simpler code and has the advantage that 1 is reduced, so there is no need to change the field.
</p>
TicketjdemeyerTue, 23 Apr 2013 09:34:11 GMTstatus changed
https://trac.sagemath.org/ticket/14472#comment:7
https://trac.sagemath.org/ticket/14472#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
TicketcremonaTue, 23 Apr 2013 09:39:58 GMT
https://trac.sagemath.org/ticket/14472#comment:8
https://trac.sagemath.org/ticket/14472#comment:8
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14472#comment:7" title="Comment 7">jdemeyer</a>:
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!
</p>
TicketjdemeyerTue, 23 Apr 2013 10:43:40 GMT
https://trac.sagemath.org/ticket/14472#comment:9
https://trac.sagemath.org/ticket/14472#comment:9
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14472#comment:8" title="Comment 8">cremona</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14472#comment:7" title="Comment 7">jdemeyer</a>:
I like the use of nfeltdiveuc, but (as you have maybe already noticed) the values of r,s,t are not the reduced values...
</p>
</blockquote>
<p>
Sorry, I don't understand what you mean. Can you say more precisely what is wrong?
</p>
TicketcremonaTue, 23 Apr 2013 10:57:44 GMT
https://trac.sagemath.org/ticket/14472#comment:10
https://trac.sagemath.org/ticket/14472#comment:10
<p>
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?
</p>
<p>
Also, I am getting an warning that line 9821 in sage.libs.pari.gen.pyx is unreachable code. Does tat need looking into?
</p>
TicketjdemeyerTue, 23 Apr 2013 11:46:51 GMTstatus changed
https://trac.sagemath.org/ticket/14472#comment:11
https://trac.sagemath.org/ticket/14472#comment:11
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketjdemeyerTue, 23 Apr 2013 11:49:36 GMTattachment set
https://trac.sagemath.org/ticket/14472
https://trac.sagemath.org/ticket/14472
<ul>
<li><strong>attachment</strong>
set to <em>trac_14472-elliptic_curves_jd.patch</em>
</li>
</ul>
TicketjdemeyerTue, 23 Apr 2013 11:50:04 GMT
https://trac.sagemath.org/ticket/14472#comment:12
https://trac.sagemath.org/ticket/14472#comment:12
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14472#comment:10" title="Comment 10">cremona</a>:
</p>
<blockquote class="citation">
<p>
Also, I am getting an warning that line 9821 in sage.libs.pari.gen.pyx is unreachable code. Does tat need looking into?
</p>
</blockquote>
<p>
That has nothing to do with this ticket, but I fixed it anyway.
</p>
TicketjdemeyerThu, 25 Apr 2013 11:54:43 GMT
https://trac.sagemath.org/ticket/14472#comment:13
https://trac.sagemath.org/ticket/14472#comment:13
<p>
John, do you want to formally review my patch? I give positive review to the parts which were copied from your initial patch.
</p>
TicketcremonaThu, 25 Apr 2013 12:26:06 GMTstatus, reviewer, author changed
https://trac.sagemath.org/ticket/14472#comment:14
https://trac.sagemath.org/ticket/14472#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Jeroen Demeyer</em> to <em>Jeroen Demeyer, John Cremona</em>
</li>
<li><strong>author</strong>
changed from <em>John Cremona</em> to <em>John Cremona, Jeroen Demeyer</em>
</li>
</ul>
<p>
I give positive review to the parts which Jeroen added, hance we have an overall positive review.
</p>
TicketjdemeyerTue, 30 Apr 2013 09:40:08 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/14472#comment:15
https://trac.sagemath.org/ticket/14472#comment:15
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.10.beta1</em>
</li>
</ul>
Ticket