Opened 12 years ago

Closed 11 years ago

#1234 closed defect (fixed)

[with patch; with positive review] analytic_rank crashes

Reported by: zimmerma Owned by: was
Priority: minor Milestone: sage-3.3
Component: algebraic geometry Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

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 was 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 12 years ago by 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 12 years ago by mabshoff

  • Milestone changed from sage-2.10 to sage-2.9.1

comment:3 Changed 12 years ago by mabshoff

Still an issue with Sage 2.10.2.alpha0.

Cheers,

Michael

comment:4 Changed 12 years ago by 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 follow-up: Changed 12 years ago by mabshoff

What shall we do about this ticket?

Thoughts?

Cheers,

Michael

comment:6 in reply to: ↑ 5 Changed 12 years ago by 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 12 years ago by 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 11 years ago by was

comment:8 Changed 11 years ago by was

  • Summary changed from analytic_rank crashes to [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 11 years ago by cremona

  • Summary changed from [with patch; needs review] analytic_rank crashes to [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 follow-up: Changed 11 years ago by 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).

comment:11 in reply to: ↑ 10 Changed 11 years ago by 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 11 years ago by mabshoff

  • Summary changed from [with patch; with positive review] analytic_rank crashes to [with patch; with positive review] analytic_rank crashes

comment:13 Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.4.1 to sage-3.3
  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.3.alpha1

Cheers,

Michael

Note: See TracTickets for help on using tickets.