Sage: Ticket #31317: eclib interface uses bad default value for elliptic curve modular symbols
https://trac.sagemath.org/ticket/31317
<p>
This was reported to me by Chris Wuthrich in December 2019, and by Barry Mazur and Karl Rubin a few days ago. I originally thought that there was a bug in eclib, but it turns out that the only bug is that the default value of certain parameters is too small for the cases they were interested in. I will post a 2-line patch to increase this. It has to be a 2-line patch, not 1, since the current eclib interfae does not expose the relevant parameter to Sage at all currently.
</p>
<p>
Here are two example. Wuthrich's (takes a little while as the conductor is 87416):
</p>
<pre class="wiki">sage: E = EllipticCurve([49,-49])
sage: me = E.modular_symbol(implementation="eclib")
sage: me(1/8)
10/17
sage: mn = E.modular_symbol(implementation="num")
sage: mn(1/8)
1/2
</pre><p>
and Mazur's:
</p>
<pre class="wiki">sage: E = EllipticCurve('1590g1')
sage: ms = E.modular_symbol()
sage: [ms(a/5) for a in [1..4]]
[1001/153, -1001/153, -1001/153, 1001/153]
sage: ms = E.modular_symbol(implementation='num')
sage: [ms(a/5) for a in [1..4]]
[13/2, -13/2, -13/2, 13/2]
</pre><p>
In both cases, after my patch the 'eclib' values agree with the numerical values.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/31317
Trac 1.1.6cremonaTue, 02 Feb 2021 10:10:24 GMTstatus changed; commit, branch set
https://trac.sagemath.org/ticket/31317#comment:1
https://trac.sagemath.org/ticket/31317#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>da09de4681c411f9e01069de4273c328a8d33b8b</em>
</li>
<li><strong>branch</strong>
set to <em>u/cremona/31317</em>
</li>
</ul>
<p>
As well as adding the nap option to te interface itself I passed this up the chain so that users can see it too, with doctests. While there I tidied up the documentation a bit too.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit?id=da09de4681c411f9e01069de4273c328a8d33b8b"><span class="icon"></span>da09de4</a></td><td><code>#31317 eclib elliptic curve modular symbols</code>
</td></tr></table>
TicketwuthrichTue, 02 Feb 2021 10:34:56 GMT
https://trac.sagemath.org/ticket/31317#comment:2
https://trac.sagemath.org/ticket/31317#comment:2
<p>
Is there evidence that 1000 is the right bound for all curves? All for which it makes sense to use eclib. Maybe it should increase with the conductor. Probably 100*sqrt(N) or so is saver.
</p>
<p>
(sorry not to reply in full, busy with marking new semester etc)
</p>
TicketcremonaTue, 02 Feb 2021 11:12:49 GMTstatus changed
https://trac.sagemath.org/ticket/31317#comment:3
https://trac.sagemath.org/ticket/31317#comment:3
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/31317#comment:2" title="Comment 2">wuthrich</a>:
</p>
<blockquote class="citation">
<p>
Is there evidence that 1000 is the right bound for all curves? All for which it makes sense to use eclib. Maybe it should increase with the conductor. Probably 100*sqrt(N) or so is safer.
</p>
</blockquote>
<p>
Good point -- though at the top level E.modular_symbol() used default nap=400 in this patch. As you know, for large conductor it will take a long time to make this construction, so I rather doubt it will be used much for N>10000 (say), so a default which works on this range seemed reasonable. On the other hand, compared with the time it takes to construct the modular symbol space, the time to compute more ap from the curve is negligible, suggesting that having a higher default is better.
</p>
<p>
I will be changing eclib's own default (which is 300 ap) anyway, probably to 1000, and the way the code works is that it sets a minimum value of nap (currently 300) so that whatever value the user gives is increased to this if less. (i.e., there is in effect the line nap=max(300,nap) in eclib's relevant function.)
</p>
<p>
I would be so much happier if it were possible to get this normalising factor another way than by taking rations of periods. It is *only* needed at all so that we get the right answers for non-optima curves -- when you give it an optimal curve it goes to all this trouble to compute the ratio as 1 (or in the example before the fix it computed it as 73/68 or something), but I know of no way to do that. It would be possible to change eclib so that it assumes that the curve is optimal by default, only computing the normalisation factors if the users specifically asks. Then on the Sage side we could decide to use the default if the curve's label ends in '1', or if the isogeny class consists of only one curve.
</p>
<p>
The current fix is designed to be easily implemented as it requires no change to eclib -- which had no bugs in this regard, except possible the too low default value of 300.
</p>
<p>
But I will take your suggestion on board with a new patch, and while waiting for the review I'll test it as many curves as I can -- the test being that 'eclib' and 'num' give the same answer for all small rationals.
</p>
<blockquote class="citation">
<p>
(sorry not to reply in full, busy with marking new semester etc)
</p>
</blockquote>
TicketgitTue, 02 Feb 2021 11:40:09 GMTcommit changed
https://trac.sagemath.org/ticket/31317#comment:4
https://trac.sagemath.org/ticket/31317#comment:4
<ul>
<li><strong>commit</strong>
changed from <em>da09de4681c411f9e01069de4273c328a8d33b8b</em> to <em>2ab86ef72a3ddc7341ed76e4dc0dbd0ce7ee0162</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=2ab86ef72a3ddc7341ed76e4dc0dbd0ce7ee0162"><span class="icon"></span>2ab86ef</a></td><td><code>#31317: more intelligent default for nap</code>
</td></tr></table>
TicketcremonaTue, 02 Feb 2021 11:41:47 GMTstatus changed
https://trac.sagemath.org/ticket/31317#comment:5
https://trac.sagemath.org/ticket/31317#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I switched the default at at the top level to max(100*N.isqrt(),10000). I have started a test for all conductors up to (however far I get, but maybe) 10000. By comparison, even for conductors up to 500000 I was only computing 6000 ap when finding the curves, which requires far greater precision.
</p>
TicketcremonaTue, 02 Feb 2021 16:32:55 GMT
https://trac.sagemath.org/ticket/31317#comment:6
https://trac.sagemath.org/ticket/31317#comment:6
<p>
For all curves of conductor up to 1000, with the new code, the 'eclib' and 'num' results agree (at all rationals with numerator and denominator bounded by 5). I'll continue to test more, but no-one need be in any doubt that this should be merged.
</p>
TicketwuthrichWed, 03 Feb 2021 08:54:41 GMTstatus changed; reviewer, author set
https://trac.sagemath.org/ticket/31317#comment:7
https://trac.sagemath.org/ticket/31317#comment:7
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Chris Wuthrich</em>
</li>
<li><strong>author</strong>
set to <em>John Cremona</em>
</li>
</ul>
<p>
I agree with all that is said here and I checked the code and all tests pass etc.
</p>
TicketcremonaWed, 03 Feb 2021 14:14:41 GMT
https://trac.sagemath.org/ticket/31317#comment:8
https://trac.sagemath.org/ticket/31317#comment:8
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/31317#comment:7" title="Comment 7">wuthrich</a>:
</p>
<blockquote class="citation">
<p>
I agree with all that is said here and I checked the code and all tests pass etc.
</p>
</blockquote>
<p>
Thanks. Although some patchbots are reporting test failures these have nothing to do with this patch, so are less than helpful. Perhaps patchbots should first run all tests without applying the patch, and only if those pass would they apply the patch and retest. That would take twice as long but would avoid spurious failures like this which probably have the effect that the release manager assumes something is actually wrong and the bug-fix never gets merged.
</p>
TicketvbraunTue, 09 Mar 2021 00:02:11 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/31317#comment:9
https://trac.sagemath.org/ticket/31317#comment:9
<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>branch</strong>
changed from <em>u/cremona/31317</em> to <em>2ab86ef72a3ddc7341ed76e4dc0dbd0ce7ee0162</em>
</li>
</ul>
Ticket