Opened 15 years ago

Closed 14 years ago

#1234 closed defect (fixed)

[with patch; with positive review] analytic_rank crashes

Reported by: Paul Zimmermann Owned by: William Stein
Priority: minor Milestone: sage-3.3
Component: algebraic geometry Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Maybe the following is not feasible, but analytic_rank could crash smoothly.

sage: d=100032426715415089/251987961355200625
sage: E = EllipticCurve([0, -d^3+5*d^2, 0, -8*d^5+8*d^4, 4*d^8-8*d^7+4*d^6])
sage: F = E.minimal_model()
sage: F.analytic_rank(algorithm='cremona')
<type 'exceptions.RuntimeError'>: Error: '  *** elltors: precision too low in torsell.

sage: F.analytic_rank(algorithm='rubinstein')
<type 'exceptions.TypeError'>: unable to convert x (= 6.90579e+20 and is too large) to an integer

sage: F.analytic_rank(algorithm='sympow')
sympow 1.018 RELEASE  (c) Mark Watkins --- see README and COPYING for details
**ERROR** c4 invariant is too large

From John Cremona: the "cremona" version just wraps a gp script, which needs to have sufficient precision even for ellinit() to work ok here. Unfortunately this call will run in its own gp session, and it is not possible for the user to set the precision. (I found the same while running a lot of examples through Denis Simon's gp scripts). The solution is to change the wrapper to have a precision parameter, with some reasonable default for backwards compatibility, which gets passes through to gp.

Attachments (1)

trac_1234.patch (4.9 KB) - added by William Stein 14 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 15 years ago by John Cremona

NB I think that all functions which wrap gp scripts should have the ability to set various gp defaults before the scripts are run. Notably the real precision (which is the culprit above), but also in other cases.

This should be easy but tedious -- is there anywhere a list of which functions operate by running a gp script?

jec

comment:2 Changed 15 years ago by Michael Abshoff

Milestone: sage-2.10sage-2.9.1

comment:3 Changed 15 years ago by Michael Abshoff

Still an issue with Sage 2.10.2.alpha0.

Cheers,

Michael

comment:4 Changed 15 years ago by John Cremona

I can only speak for the first of these, begin responsible for the gp script which computes analytic rank.

This curve has conductor N=690579402364042119390 which is vastly too big for the analytic rank algorithm anyway (it requires the computation of O(sqrt(N)) terms of the L-series).

Also relevant here is that my analytic rank gp code is *old*. When Magma adopted it, it was vastly improved (by Mark Watkins), which means that for several years I have not used my own gp program at all, let alone developed it. For this curve, I don't think even Magma would succeed in computing its analytic rank.

comment:5 Changed 14 years ago by Michael Abshoff

What shall we do about this ticket?

Thoughts?

Cheers,

Michael

comment:6 in reply to:  5 Changed 14 years ago by John Cremona

Replying to mabshoff:

What shall we do about this ticket?

Thoughts?

Cheers,

Michael

Given the fundamental problem that computing the analytic rank increases rapidly with the conductor N (I think it is sqrt(N)), one solution would be to impose a (carefully-chosen but necessarily somewhat arbitrary) cutoff N_max, so that asking for the analytic rank of a curve of conductor>N_max would result in an error.

One way to implement this would be to have N_max a parameter to the analytic rank function, with a default value of (say) 10^6 or 10^7. (I would have to do some experiments to decide on a sensible value). The docstring could explain that the user is allowed to increase this but warn that it may take (effectively) for ever.

comment:7 Changed 14 years ago by John Cremona

PS The vale of N_max would presumably be different for the different algorithms supported (sympow, lcalc, etc). I was only thing of the "cremona" algorithm. I also note that the docstring claims that lcalc is the most efficient, but cremona is the default -- that was not my decision!

Changed 14 years ago by William Stein

Attachment: trac_1234.patch added

comment:8 Changed 14 years ago by William Stein

Summary: analytic_rank crashes[with patch; needs review] analytic_rank crashes

The attached patch cleans up the exceptions raised, as requested. Also, in the (default) case where Cremona's gp script is used, the precision is automatically doubled until it doesn't fail. I also start the precision at 16 rather than the default, since it will get automatically double if necessary, and it's about 3 times faster usually by using this smaller precision to start:

BEFORE:
sage: E = EllipticCurve('5077a')
sage: time E.analytic_rank()
CPU times: user 0.01 s, sys: 0.01 s, total: 0.02 s
Wall time: 0.21 s


AFTER:
sage: E = EllipticCurve('5077a')
sage: time E.analytic_rank()
CPU times: user 0.02 s, sys: 0.00 s, total: 0.02 s
Wall time: 0.06 s


and another

BEFORE:
sage: time elliptic_curves.rank(4)[0].analytic_rank()
CPU times: user 0.01 s, sys: 0.00 s, total: 0.01 s
Wall time: 0.50 s
4

AFTER:
sage: time elliptic_curves.rank(4)[0].analytic_rank()
CPU times: user 0.01 s, sys: 0.00 s, total: 0.01 s
Wall time: 0.33 s
4

comment:9 Changed 14 years ago by John Cremona

Summary: [with patch; needs review] analytic_rank crashes[with patch; with positive review] analytic_rank crashes

Review: excellent and well done. Applies cleanly to 3.3.alpha0 and all tests in elliptic_curves pass.

Suggestion: I see that for EllipticCurve?([1234567,89101112]) which has conductor 61928339435779485920, sympow and rubinstein know they are beaten and quit, while cremona just takes a long time (I did not wait). It might be a good idea to work out a sensible conductor limit for my gp script and have sage only call the script if under that (perhaps with a parameter to allow users to override it). But that would depend on many factors (speed of machine etc), and this patch should not be delayed because of that.

Pass!

comment:10 Changed 14 years ago by Paul Zimmermann

I also was testing the patch, but John was faster than me. Just a comment: it would be better to capitalize people names (rubinstein -> Rubinstein, weierstrass -> Weierstrass), at least in the documentation (for the options, it might involve too much work).

comment:11 in reply to:  10 Changed 14 years ago by John Cremona

Replying to zimmerma:

I also was testing the patch, but John was faster than me. Just a comment: it would be better to capitalize people names (rubinstein -> Rubinstein, weierstrass -> Weierstrass), at least in the documentation (for the options, it might involve too much work).

I agree with capitalization in documentation; for parameters it would probably be best to allow either.

comment:12 Changed 14 years ago by Michael Abshoff

Summary: [with patch; with positive review] analytic_rank crashes[with patch; with positive review] analytic_rank crashes

comment:13 Changed 14 years ago by Michael Abshoff

Milestone: sage-3.4.1sage-3.3
Resolution: fixed
Status: newclosed

Merged in Sage 3.3.alpha1

Cheers,

Michael

Note: See TracTickets for help on using tickets.