Sage: Ticket #12080: manin constant
https://trac.sagemath.org/ticket/12080
<p>
My definition of the Manin constant of an elliptic curve E/Q is the rational number c such that
</p>
<blockquote>
<p>
phi(omega) = c * f dq/q
</p>
</blockquote>
<p>
where
</p>
<ul><li>phi is the modular parametrisation X_0(N) -> E of minimal degre
</li><li>omega is the Neron differential on E
</li><li>f is the normalised newform
</li><li>q is q=exp(2pi i tau) as usual
</li></ul><p>
With this definition I get a different answer than sage. For instance for 11a2, I get 1 not 5.
</p>
<p>
Either one has to change the implementation or one has to add to the documentation the definition of what is computed.
</p>
<p>
The current implementation (from <a class="closed ticket" href="https://trac.sagemath.org/ticket/5138" title="enhancement: [with patch; positive review] implement computing manin constants of ... (closed: fixed)">#5138</a> ) computes the minimal degree of an isogeny from E to the X_0-optimal curve E_0 and multiplies the manin constant of E_0 by this degree. Instead, with my definition, we have to multiply with the number c' where psi*(omega) = c' * omega_0 with psi the isogeny E_0 -> E of minimal degree and omega_0 the Neron differential of E_0. This c' is a divisor of the degree of psi, but on many occasions it is 1.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/sage_logo_trac_v2.png
https://trac.sagemath.org/ticket/12080
Trac 1.1.6wuthrichThu, 24 Nov 2011 11:50:20 GMT
https://trac.sagemath.org/ticket/12080#comment:1
https://trac.sagemath.org/ticket/12080#comment:1
<p>
The following function, which I wrote for another thing, computes the valuation of c' as above. So this may be useful if it is decided that the implementation rather than the documentation of <code>manin_constant</code> needs changing.
</p>
<pre class="wiki">def neron_scaling(phi,v):
r"""
Given an isogeny `\phi` between elliptic curves over a number field and a place `v`,
this gives back the valuation at `v` of the constant c such that
`\phi*(\omega') = c \cdot \omega`
where `\omega` and `\omega'` are Neron differentials.
INPUT:
- ``phi`` - an isogeny between elliptic curves over a number field
- ``v`` - a finite place of this field
OUTPUT: an integer
Note: This only makes sense if ``v`` devides the degree of ``phi``.
EXAMPLES::
sage: E = EllipticCurve("20a1")
sage: P= E.lift_x(0)
sage: P.order()
3
sage: neron_scaling(phihat,3)
1
sage: neron_scaling(phi,3)
0
sage: E = EllipticCurve("11a1")
sage: E = E.change_weierstrass_model([5,1,0,0])
sage: E2 = EllipticCurve("11a2").change_weierstrass_model([1/25,0,1,0])
sage: P = E.torsion_points()[2]
sage: phi = E.isogeny(P, codomain=E2)
sage: neron_scaling(phi,5)
0
sage: neron_scaling(phi.dual(),5)
1
"""
E = phi.domain()
E2 = phi.codomain()
Emin = E.local_data(v).minimal_model()
E2min = E2.local_data(v).minimal_model()
u = E.isomorphism_to(Emin).u
u2 = E2.isomorphism_to(E2min).u
a1 = phi.formal()[1]
return valuation(a1,v) - valuation(u,v) + valuation(u2,v)
</pre>
TicketcremonaThu, 24 Nov 2011 12:01:25 GMT
https://trac.sagemath.org/ticket/12080#comment:2
https://trac.sagemath.org/ticket/12080#comment:2
<p>
Interesting. All Sage does is call sympow (Mark Watkins's program). Magma gives the same output as Sage (not surprising since it also uses MW's code). Have you tried asking MW for his rationale?
</p>
TicketwuthrichThu, 24 Nov 2011 12:16:34 GMT
https://trac.sagemath.org/ticket/12080#comment:3
https://trac.sagemath.org/ticket/12080#comment:3
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12080#comment:2" title="Comment 2">cremona</a>:
</p>
<blockquote class="citation">
<p>
Interesting. All Sage does is call sympow (Mark Watkins's program).
</p>
</blockquote>
<p>
Does it ?
I see these lines in the code....
</p>
<pre class="wiki">label = self.cremona_label()
optimal = self.optimal_curve()
if optimal == self:
return Integer(1)
L, v = self._shortest_paths()
i = L.index(optimal)
return v[i]
</pre><p>
and <code>shortest_path</code> is what you think it is.
</p>
TicketwuthrichThu, 24 Nov 2011 12:20:16 GMT
https://trac.sagemath.org/ticket/12080#comment:4
https://trac.sagemath.org/ticket/12080#comment:4
<p>
magma, by the way, gets the correct answer (meaning the one consistent with my definition above).
</p>
<pre class="wiki">> E := EllipticCurve(CremonaDatabase(), "11a2");
> ManinConstant(E);
1
> E := EllipticCurve(CremonaDatabase(), "11a3");
> ManinConstant(E);
5
> E := EllipticCurve(CremonaDatabase(), "11a1");
> ManinConstant(E);
1
</pre>
TicketcremonaThu, 24 Nov 2011 13:45:56 GMT
https://trac.sagemath.org/ticket/12080#comment:5
https://trac.sagemath.org/ticket/12080#comment:5
<p>
Now I am confused. First, the code I see for E.modular_degree() in 4.7.2 looks like:
</p>
<pre class="wiki"> try:
return self.__modular_degree
except AttributeError:
if algorithm == 'sympow':
from sage.lfunctions.all import sympow
m = sympow.modular_degree(self)
elif algorithm == 'magma':
from sage.interfaces.all import magma
m = rings.Integer(magma(self).ModularDegree())
else:
raise ValueError, "unknown algorithm %s"%algorithm
self.__modular_degree = m
return m
</pre><p>
but I admit that I did not look to see what sympow.modular_degree() does.
</p>
<p>
Secondly, in Magma V2.17-9 I get
</p>
<pre class="wiki">> ModularDegree(EllipticCurve("11a2"));
5
</pre>
TicketwuthrichThu, 24 Nov 2011 14:09:20 GMT
https://trac.sagemath.org/ticket/12080#comment:6
https://trac.sagemath.org/ticket/12080#comment:6
<p>
I am talking about the <strong>MANIN CONSTANT</strong> not the MODULAR DEGREE !
</p>
<p>
The Manin constant as I define it in the description is not the degree, e.g. for 11a2 we have an isogeny X_0(11) = 11a1 -> 11a2 of degree 5, but the pullback of the Neron differential is the Neron differential as the map is etale with kernel Z/5Z.
</p>
<p>
The above code is in <code>manin_constant</code> and was added in <a class="closed ticket" href="https://trac.sagemath.org/ticket/5138" title="enhancement: [with patch; positive review] implement computing manin constants of ... (closed: fixed)">#5138</a>.
</p>
TicketwasThu, 24 Nov 2011 16:09:30 GMT
https://trac.sagemath.org/ticket/12080#comment:7
https://trac.sagemath.org/ticket/12080#comment:7
<p>
Chris, you're right: the code in sage should change, and docs be improved. I was sloppy.
</p>
TicketwuthrichMon, 28 Nov 2011 18:40:38 GMTattachment set
https://trac.sagemath.org/ticket/12080
https://trac.sagemath.org/ticket/12080
<ul>
<li><strong>attachment</strong>
set to <em>trac_12080_manin_constant.patch</em>
</li>
</ul>
<p>
patch exported against 4.7.2
</p>
TicketwuthrichMon, 28 Nov 2011 18:45:09 GMTstatus changed; author set
https://trac.sagemath.org/ticket/12080#comment:8
https://trac.sagemath.org/ticket/12080#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>wuthrich</em>
</li>
</ul>
<p>
Here is a patch for the problem. Now, the algorithm is based on comparing the period lattices for the elliptic curve and the optimal curve in the isogeny class.
</p>
<p>
I checked this implementation against the one in magma. for conductors below 1000, there are two exceptions, 27a4 and 80b4. I included them as doctests. I believe that this patch returns the correct value and magma has a small bug in its <a class="missing wiki">ManinConstant?</a> function.
</p>
<p>
So the reviewer is asked to compute by hand the Manin constant for these two curves and confirm that the returned value is indeed correct. Then I will send a bug report to magma.
</p>
<p>
Furthermore, a precise question to John : Is it true (as the algorithm here assumes) that you have checked that the Manin constant is 1 for all optimal curves in your table ?
</p>
TicketwasMon, 28 Nov 2011 19:46:33 GMT
https://trac.sagemath.org/ticket/12080#comment:9
https://trac.sagemath.org/ticket/12080#comment:9
<p>
Wuthrich: the appendix by Cremona to this paper is about him verifying that the Manin constant is 1:
<a class="ext-link" href="http://wstein.org/papers/ars-manin/"><span class="icon"></span>http://wstein.org/papers/ars-manin/</a>
</p>
TicketwuthrichMon, 28 Nov 2011 20:32:29 GMT
https://trac.sagemath.org/ticket/12080#comment:10
https://trac.sagemath.org/ticket/12080#comment:10
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12080#comment:9" title="Comment 9">was</a>:
</p>
<blockquote class="citation">
<p>
Wuthrich: the appendix by Cremona to this paper is about him verifying that the Manin constant is 1:
<a class="ext-link" href="http://wstein.org/papers/ars-manin/"><span class="icon"></span>http://wstein.org/papers/ars-manin/</a>
</p>
</blockquote>
<p>
Yes, indeed. I was referring to this. But at the time the table was smaller, so I jsut wanted to check that the assumption underlying the algorithm is known to hold for all curves in the tale (now and in the future).
</p>
<p>
I should maybe include the paper as the basic reference in the docstring....
</p>
TicketcremonaMon, 28 Nov 2011 22:50:59 GMT
https://trac.sagemath.org/ticket/12080#comment:11
https://trac.sagemath.org/ticket/12080#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12080#comment:10" title="Comment 10">wuthrich</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12080#comment:9" title="Comment 9">was</a>:
</p>
<blockquote class="citation">
<p>
Wuthrich: the appendix by Cremona to this paper is about him verifying that the Manin constant is 1:
<a class="ext-link" href="http://wstein.org/papers/ars-manin/"><span class="icon"></span>http://wstein.org/papers/ars-manin/</a>
</p>
</blockquote>
<p>
Yes, indeed. I was referring to this. But at the time the table was smaller, so I jsut wanted to check that the assumption underlying the algorithm is known to hold for all curves in the tale (now and in the future).
</p>
</blockquote>
<p>
I will look into this, as I have been meaning to anyway. There was some extra work to do when I wrote that appendix, and so there may also be some to do to verify that c=1 for the extended tables. Some of this is automatically done on the fly, but not all. It *is* true that when there is only one curve in the isogeny class that c=1 -- this is not as trivial a statement as it appears, but it amounts to saying that the optimal curve is minimal, and I do check that on the fly.
</p>
<blockquote class="citation">
<p>
I should maybe include the paper as the basic reference in the docstring....
</p>
</blockquote>
TicketwasThu, 22 Mar 2012 12:46:55 GMTstopgaps set
https://trac.sagemath.org/ticket/12080#comment:12
https://trac.sagemath.org/ticket/12080#comment:12
<ul>
<li><strong>stopgaps</strong>
set to <em>todo</em>
</li>
</ul>
TicketwasThu, 29 Mar 2012 12:48:50 GMT
https://trac.sagemath.org/ticket/12080#comment:13
https://trac.sagemath.org/ticket/12080#comment:13
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12080#comment:4" title="Comment 4">wuthrich</a>:
</p>
<blockquote class="citation">
<p>
magma, by the way, gets the correct answer (meaning the one consistent with my definition above).
</p>
</blockquote>
<p>
I finally wrote the obvious program that one should write when ref'ing this patch:
</p>
<pre class="wiki">wstein@boxen:/tmp/wstein/sage-5.0.b9$ more 12080.sage
for E in cremona_curves([1..10000]):
N = E.conductor()
if N%100==0: print N
c = E.manin_constant()
d = magma(E).ManinConstant()
if c != d:
print E.cremona_label(), c, d
</pre><p>
When running it up to 300, I get two differences from Magma:
</p>
<pre class="wiki">27a4 3 1
80b4 2 1
nothing else up to 300
</pre><p>
It could very well be that Magma is wrong... but as part of this patch, could somebody get to the bottom of this?
</p>
TicketwasThu, 29 Mar 2012 12:50:06 GMT
https://trac.sagemath.org/ticket/12080#comment:14
https://trac.sagemath.org/ticket/12080#comment:14
<p>
I've asked Mark Watkins to look at this, since he wrote <a class="missing wiki">ManinConstant?</a> in Magma....
</p>
TicketwasThu, 29 Mar 2012 13:09:58 GMTstatus changed
https://trac.sagemath.org/ticket/12080#comment:15
https://trac.sagemath.org/ticket/12080#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Mark Watkins says: "Sage is definitely correct. Sympow code I wrote agrees with Sage here. Magma has a bug."
</p>
<p>
So... positive review. (Also, I checked up to 1200 and Sage and Magma agree in the rest of the cases.)
</p>
<p>
Incidentally, Magma's code is way faster:
</p>
<pre class="wiki">sage: E = list(cremona_curves([1200]))[0]
sage: E
Elliptic Curve defined by y^2 = x^3 - x^2 + 17*x - 38 over Rational Field
sage: time E.manin_constant()
1
Time: CPU 0.71 s, Wall: 0.82 s
sage: F = magma(E)
sage: time F.ManinConstant()
1
Time: CPU 0.00 s, Wall: 0.06 s
</pre><p>
However, according to Mark Watkins, Magma is fast because it assumes <strong>every</strong> imaginable conjecture in that implementation (and doesn't actually compute any periods, etc.)
</p>
TicketjdemeyerThu, 29 Mar 2012 14:56:02 GMTauthor changed; reviewer set; stopgaps deleted
https://trac.sagemath.org/ticket/12080#comment:16
https://trac.sagemath.org/ticket/12080#comment:16
<ul>
<li><strong>reviewer</strong>
set to <em>William Stein</em>
</li>
<li><strong>author</strong>
changed from <em>wuthrich</em> to <em>Chris Wuthrich</em>
</li>
<li><strong>stopgaps</strong>
<em>todo</em> deleted
</li>
</ul>
TicketjdemeyerMon, 02 Apr 2012 15:25:01 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/12080#comment:17
https://trac.sagemath.org/ticket/12080#comment:17
<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.0.beta12</em>
</li>
</ul>
Ticket