Sage: Ticket #14476: non-integral models can cause a bug in local_data for elliptic curves over number fields
https://trac.sagemath.org/ticket/14476
<p>
This example was spotted by Dino Lorenzini:
</p>
<pre class="wiki">sage: t = QQ['t'].0
sage: K.<g> = NumberField(t^4 - t^3-3*t^2 - t +1)
sage: x=-3/5*g^3 + 2/5*g^2 + 3/5*g + 4/5
sage: y=1/15*g^3 - 3/5*g^2 + 3/5*g + 17/15
sage: r = (x^2*y - x*y + y - 1) / (x^2*y - x)
sage: s = (x*y - y + 1) / (x*y)
sage: c = s*(r - 1)
sage: b = c*r
sage: E = EllipticCurve(K,[1-c,-b,-b,0,0])
sage: E.local_data()
</pre><p>
which causes a <code>TypeError</code> in <code>_reduce_local</code>.
Tickets that may be linked to this are <a class="closed ticket" href="https://trac.sagemath.org/ticket/11630" title="defect: Local data of elliptic curves should not do any global work (closed: fixed)">#11630</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/9410" title="defect: EC.local_data() can't handle extensions of number fields (closed: wontfix)">#9410</a>.
</p>
<p>
<strong>Added later:</strong> Moreover I can add another bug that he showed me
</p>
<pre class="wiki">sage: R.<t> = QQ[]
sage: K.<g> = NumberField(t^4 - t^3 - 3*t^2 - t + 1)
sage: E = EllipticCurve([ -43/625*g^3 + 14/625*g^2 - 4/625*g + 706/625, -4862/78125*g^3 - 4074/78125*g^2 - 711/78125*g + 10304/78125, -4862/78125*g^3 - 4074/78125*g^2 - 711/78125*g + 10304/78125, 0,0])
sage: E.global_integral_model()
</pre><p>
ends in a <code>AssertionError: bug in global_integral_model</code>.
</p>
<p>
APPLY:
</p>
<ul><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/14476/trac_14476_bugs_in_local_data.patch" title="Attachment 'trac_14476_bugs_in_local_data.patch' in Ticket #14476">trac_14476_bugs_in_local_data.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/14476/trac_14476_bugs_in_local_data.patch" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/14476/trac_14476_trac_links.patch" title="Attachment 'trac_14476_trac_links.patch' in Ticket #14476">trac_14476_trac_links.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/14476/trac_14476_trac_links.patch" title="Download"></a>
</li></ul><p>
<strong>NOTE:</strong> 32-bit doctests require <a class="closed ticket" href="https://trac.sagemath.org/ticket/15243" title="enhancement: Change algorithm for K.uniformizer(P) (closed: fixed)">#15243</a> to be applied on top of these patches.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/14476
Trac 1.1.6cremonaMon, 22 Apr 2013 21:09:00 GMT
https://trac.sagemath.org/ticket/14476#comment:1
https://trac.sagemath.org/ticket/14476#comment:1
<p>
The reason is this:
</p>
<pre class="wiki">sage: E.is_global_integral_model()
False
</pre><p>
and if one replaces E by a global integral model then all is well:
</p>
<pre class="wiki">sage: E1 = E.global_integral_model()
sage: E1.local_data()
[Local data at Fractional ideal (g^3 - g^2 - 2*g - 1):
Reduction type: bad split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 11
Conductor exponent: 1
Kodaira Symbol: I11
Tamagawa Number: 11,
Local data at Fractional ideal (-g^2 + g + 1):
Reduction type: bad split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 11
Conductor exponent: 1
Kodaira Symbol: I11
Tamagawa Number: 11,
Local data at Fractional ideal (g^2 - g - 2):
Reduction type: bad split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 11
Conductor exponent: 1
Kodaira Symbol: I11
Tamagawa Number: 11,
Local data at Fractional ideal (3*g^3 - 12*g - 4):
Reduction type: bad non-split multiplicative
Local minimal model: Elliptic Curve defined by y^2 + (g^3+g)*x*y + g*y = x^3 + (-g^2+1)*x^2 + (126914883*g^3-73346242*g^2-411702677*g-300687331)*x + (-1068031359960*g^3+617234077764*g^2+3464617746565*g+2530385673583) over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Minimal discriminant valuation: 1
Conductor exponent: 1
Kodaira Symbol: I1
Tamagawa Number: 1]
</pre><p>
Of course it would be possible to have local_data() do this. At the very least, local data should give a more polite message.
</p>
TicketwuthrichSat, 04 May 2013 22:49:50 GMT
https://trac.sagemath.org/ticket/14476#comment:2
https://trac.sagemath.org/ticket/14476#comment:2
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14476#comment:1" title="Comment 1">cremona</a>:
</p>
<blockquote class="citation">
<p>
Of course it would be possible to have local_data() do this. At the very least, local data should give a more polite message.
</p>
</blockquote>
<p>
Not only that. The documentation says that non-integral model are allowed. And I firmly believe they should be. I am not in favour of making the model globally integral beforehand, again because it should stay a local algorithm.
</p>
TicketwuthrichSat, 04 May 2013 22:54:53 GMT
https://trac.sagemath.org/ticket/14476#comment:3
https://trac.sagemath.org/ticket/14476#comment:3
<p>
Just so that I don't forget what I found out so far :
</p>
<p>
The issue, as silly as it sounds, comes up from <code>__repr__</code> this class. This does not display <code>_Emin</code> the minimal equation that Tate's algorithm found, instead it wants to compute <code>minimal_model</code>. This does nothing but calling <code>_reduced_model</code> in <code>ell_number_fields.py</code>. And that is global again. So in <code>_reduce_model</code> it checks if the coefficients are integers, while we only need that they are local integers here.
</p>
<p>
The quick solution I see is to add to the local class a function to reduce the model locally. But I will have to check what that is going to break.
</p>
TicketwuthrichSun, 05 May 2013 18:38:59 GMTdescription changed
https://trac.sagemath.org/ticket/14476#comment:4
https://trac.sagemath.org/ticket/14476#comment:4
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/14476?action=diff&version=4">diff</a>)
</li>
</ul>
TicketwuthrichSun, 05 May 2013 18:39:30 GMTdescription changed
https://trac.sagemath.org/ticket/14476#comment:5
https://trac.sagemath.org/ticket/14476#comment:5
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/14476?action=diff&version=5">diff</a>)
</li>
</ul>
TicketwuthrichSun, 05 May 2013 19:52:34 GMT
https://trac.sagemath.org/ticket/14476#comment:6
https://trac.sagemath.org/ticket/14476#comment:6
<p>
OK, I solved the second problem in <code>global_integral_model</code>. The algorithm did not check that e was negative - for indeed a uniformiser for one place could be one for another and then the non-integrality at the later was already done. Rather than just adding <code>if e<0:</code> I decided to rewrite it to make it do a minimal number of changes. Patch to come.
</p>
TicketwuthrichSun, 05 May 2013 20:58:53 GMT
https://trac.sagemath.org/ticket/14476#comment:7
https://trac.sagemath.org/ticket/14476#comment:7
<p>
I changed my mind about the first problem. There is no meaning to a local version of reduced equation. So in order not to break anything, I propose to reduce the local minimal equation only if it is globally integral coefficients.
</p>
<p>
The attached patch was exported from 5.9 after having applied
the patched for <a class="closed ticket" href="https://trac.sagemath.org/ticket/12509" title="defect: computation of height of point on elliptic curve over Q(sqrt(5)) is WRONG (closed: fixed)">#12509</a> <a class="closed ticket" href="https://trac.sagemath.org/ticket/13953" title="defect: (non)archimedian_local_height of a torsion points always gives 0 (closed: fixed)">#13953</a> <a class="closed ticket" href="https://trac.sagemath.org/ticket/11630" title="defect: Local data of elliptic curves should not do any global work (closed: fixed)">#11630</a> in that order. It solves the two above issues.
</p>
TicketwuthrichSun, 05 May 2013 20:59:12 GMTattachment set
https://trac.sagemath.org/ticket/14476
https://trac.sagemath.org/ticket/14476
<ul>
<li><strong>attachment</strong>
set to <em>trac_14476_bugs_in_local_data.patch</em>
</li>
</ul>
<p>
The attached patch was exported from 5.9 after having applied the patched for <a class="closed ticket" href="https://trac.sagemath.org/ticket/12509" title="defect: computation of height of point on elliptic curve over Q(sqrt(5)) is WRONG (closed: fixed)">#12509</a> <a class="closed ticket" href="https://trac.sagemath.org/ticket/13953" title="defect: (non)archimedian_local_height of a torsion points always gives 0 (closed: fixed)">#13953</a> <a class="closed ticket" href="https://trac.sagemath.org/ticket/11630" title="defect: Local data of elliptic curves should not do any global work (closed: fixed)">#11630</a> in that order. I
</p>
TicketwuthrichSun, 05 May 2013 21:00:09 GMTstatus changed; author set
https://trac.sagemath.org/ticket/14476#comment:8
https://trac.sagemath.org/ticket/14476#comment:8
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Chris Wuthrich</em>
</li>
</ul>
TicketchapotonSat, 18 May 2013 18:47:48 GMTattachment set
https://trac.sagemath.org/ticket/14476
https://trac.sagemath.org/ticket/14476
<ul>
<li><strong>attachment</strong>
set to <em>trac_14476_trac_links.patch</em>
</li>
</ul>
TicketchapotonSat, 18 May 2013 18:49:07 GMT
https://trac.sagemath.org/ticket/14476#comment:9
https://trac.sagemath.org/ticket/14476#comment:9
<p>
I have added a review patch that adds all missing links to trac (using the trac role <code>:trac:</code>)
</p>
TicketwuthrichMon, 20 May 2013 13:56:35 GMT
https://trac.sagemath.org/ticket/14476#comment:10
https://trac.sagemath.org/ticket/14476#comment:10
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14476#comment:9" title="Comment 9">chapoton</a>:
</p>
<blockquote class="citation">
<p>
I have added a review patch that adds all missing links to trac (using the trac role <code>:trac:</code>)
</p>
</blockquote>
<p>
Thanks. Can you afterwards give a positive review ?
</p>
TicketjdemeyerTue, 13 Aug 2013 15:35:53 GMTmilestone changed
https://trac.sagemath.org/ticket/14476#comment:11
https://trac.sagemath.org/ticket/14476#comment:11
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.11</em> to <em>sage-5.12</em>
</li>
</ul>
TicketchapotonTue, 03 Sep 2013 19:28:14 GMT
https://trac.sagemath.org/ticket/14476#comment:12
https://trac.sagemath.org/ticket/14476#comment:12
<p>
I think this ticket only needs a "mathematical green light" to get a positive review. As I am not an elliptic guy, I would prefer if some expert would enter here and say let's go.
</p>
TicketcremonaTue, 03 Sep 2013 19:41:20 GMT
https://trac.sagemath.org/ticket/14476#comment:13
https://trac.sagemath.org/ticket/14476#comment:13
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14476#comment:12" title="Comment 12">chapoton</a>:
</p>
<blockquote class="citation">
<p>
I think this ticket only needs a "mathematical green light" to get a positive review. As I am not an elliptic guy, I would prefer if some expert would enter here and say let's go.
</p>
</blockquote>
<p>
I am happy to give this the required green light -- have not tested, but I understand the problem and the solution looks good. So as long as the patches apply to 5.12.beta4 and tests pass, I am happy with it getting positive review.
</p>
TicketchapotonWed, 04 Sep 2013 07:45:03 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/14476#comment:14
https://trac.sagemath.org/ticket/14476#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Frédéric Chapoton, John Cremona</em>
</li>
</ul>
<p>
ok, then positive review.
</p>
TicketjdemeyerThu, 26 Sep 2013 13:26:40 GMTstatus changed
https://trac.sagemath.org/ticket/14476#comment:15
https://trac.sagemath.org/ticket/14476#comment:15
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
Which patch(es) should be applied?
</p>
TicketchapotonThu, 26 Sep 2013 13:57:54 GMTstatus, description changed
https://trac.sagemath.org/ticket/14476#comment:16
https://trac.sagemath.org/ticket/14476#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/14476?action=diff&version=16">diff</a>)
</li>
</ul>
<p>
both patches should be applied, as listed now at bottom of the description
</p>
TicketjdemeyerMon, 30 Sep 2013 13:14:08 GMTstatus changed
https://trac.sagemath.org/ticket/14476#comment:17
https://trac.sagemath.org/ticket/14476#comment:17
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Does the algorithm depend on choices? On arando (32-bit Ubuntu 13.04):
</p>
<pre class="wiki">sage -t --long devel/sage/sage/schemes/elliptic_curves/ell_number_field.py
**********************************************************************
File "devel/sage/sage/schemes/elliptic_curves/ell_number_field.py", line 605, in sage.schemes.elliptic_curves.ell_number_field.EllipticCurve_number_field.global_integral_model
Failed example:
E.global_integral_model()
Expected:
Elliptic Curve defined by y^2 + (-18*g^3+29*g^2+63*g+7)*x*y + (-704472*g^3-958584*g^2-166242*g+298101)*y = x^3 + (-2859*g^3-3978*g^2-669*g+1332)*x^2 over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
Got:
Elliptic Curve defined by y^2 + (-5*g^3+g^2+9*g+5)*x*y + (57305*g^3+78489*g^2+14025*g-24161)*y = x^3 + (-541*g^3-738*g^2-126*g+232)*x^2 over Number Field in g with defining polynomial t^4 - t^3 - 3*t^2 - t + 1
**********************************************************************
</pre>
TicketjdemeyerMon, 30 Sep 2013 13:21:23 GMT
https://trac.sagemath.org/ticket/14476#comment:18
https://trac.sagemath.org/ticket/14476#comment:18
<p>
Indeed, the uniformizer is not unique:
</p>
<p>
On 64-bit:
</p>
<pre class="wiki">sage: K.<t> = NumberField(x^4 - x^3 - 3*x^2 - x + 1)
sage: [K.uniformizer(P) for P,e in factor(K.ideal(5))]
[t^2 - t + 1, t^3 - t^2 - 4*t - 1, -t^3 + 2*t^2 + 3*t - 1]
</pre><p>
On 32-bit:
</p>
<pre class="wiki">sage: K.<t> = NumberField(x^4 - x^3 - 3*x^2 - x + 1)
sage: [K.uniformizer(P) for P,e in factor(K.ideal(5))]
[t^2 - t + 1, t^2 - t - 1, -t^3 + 3*t + 2]
</pre>
TicketjdemeyerMon, 30 Sep 2013 13:38:24 GMT
https://trac.sagemath.org/ticket/14476#comment:19
https://trac.sagemath.org/ticket/14476#comment:19
<p>
I think the best solution is to "fix" the <code>uniformizer</code> function, I'm thinking about a patch...
</p>
TicketjdemeyerMon, 30 Sep 2013 13:47:53 GMT
https://trac.sagemath.org/ticket/14476#comment:20
https://trac.sagemath.org/ticket/14476#comment:20
<p>
See <a class="closed ticket" href="https://trac.sagemath.org/ticket/15243" title="enhancement: Change algorithm for K.uniformizer(P) (closed: fixed)">#15243</a>.
</p>
TicketjdemeyerMon, 30 Sep 2013 15:04:16 GMTdependencies set
https://trac.sagemath.org/ticket/14476#comment:21
https://trac.sagemath.org/ticket/14476#comment:21
<ul>
<li><strong>dependencies</strong>
set to <em>#15243</em>
</li>
</ul>
TicketjdemeyerMon, 30 Sep 2013 15:17:24 GMTstatus, description changed
https://trac.sagemath.org/ticket/14476#comment:22
https://trac.sagemath.org/ticket/14476#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/14476?action=diff&version=22">diff</a>)
</li>
</ul>
TicketjdemeyerMon, 30 Sep 2013 15:17:36 GMTmilestone changed
https://trac.sagemath.org/ticket/14476#comment:23
https://trac.sagemath.org/ticket/14476#comment:23
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.12</em> to <em>sage-5.13</em>
</li>
</ul>
TicketjdemeyerMon, 30 Sep 2013 15:24:46 GMTdependencies changed
https://trac.sagemath.org/ticket/14476#comment:24
https://trac.sagemath.org/ticket/14476#comment:24
<ul>
<li><strong>dependencies</strong>
changed from <em>#15243</em> to <em>merge with #15243</em>
</li>
</ul>
TicketcremonaMon, 30 Sep 2013 15:45:48 GMT
https://trac.sagemath.org/ticket/14476#comment:25
https://trac.sagemath.org/ticket/14476#comment:25
<p>
Thanks, I think that is the best option. I am looking at your patch on <a class="closed ticket" href="https://trac.sagemath.org/ticket/15243" title="enhancement: Change algorithm for K.uniformizer(P) (closed: fixed)">#15243</a> now. I wonder if there is any chance of persuading Karim et al to make all of pari independent of 32/64 bit. Whatever reasons there were are surely no longer valid!
</p>
TicketjdemeyerWed, 02 Oct 2013 06:36:05 GMTmilestone changed
https://trac.sagemath.org/ticket/14476#comment:26
https://trac.sagemath.org/ticket/14476#comment:26
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.13</em> to <em>sage-pending</em>
</li>
</ul>
TicketjdemeyerWed, 02 Oct 2013 11:16:39 GMTstatus, milestone changed; resolution, merged set
https://trac.sagemath.org/ticket/14476#comment:27
https://trac.sagemath.org/ticket/14476#comment:27
<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.13.beta0</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-pending</em> to <em>sage-5.13</em>
</li>
</ul>
Ticket