Opened 11 years ago

Closed 10 years ago

#12565 closed defect (fixed)

E = EllipticCurve('10a1') gives a stupid traceback (rather than a smart one)

Reported by: William Stein Owned by: John Cremona
Priority: trivial Milestone: sage-5.5
Component: elliptic curves Keywords:
Cc: Merged in: sage-5.5.beta2
Authors: R. Andrew Ohana Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12341 Stopgaps:

Status badges

Description (last modified by John Cremona)

I don't like this error:

sage: E = EllipticCurve('10a1')
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/Users/wstein/sage/install/sage-5.0.beta2/devel/sage-git/sage/modular/modsym/padic_lseries/<ipython console> in <module>()

/Users/wstein/sage/install/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/schemes/elliptic_curves/constructor.pyc in EllipticCurve(x, y, j)
    293         
    294     if isinstance(x, str):
--> 295         return ell_rational_field.EllipticCurve_rational_field(x)
    296         
    297     if rings.is_RingElement(x) and y is None:

/Users/wstein/sage/install/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in __init__(self, ainvs, extra)
    197             label = ainvs
    198             X = sage.databases.cremona.CremonaDatabase()[label]
--> 199             EllipticCurve_number_field.__init__(self, Q, list(X.a_invariants()))
    200             self.__np = {}
    201             self.__gens = {}

AttributeError: 'NoneType' object has no attribute 'a_invariants'

I would expect something like:

ValueError: no known curve with Cremona label '10a1' 

Another bug. If you use Sage for a level outside the Cremona range and you're using a system-wide Sage install, this is the error you get, which is pretty scary looking:

sage: E = EllipticCurve('10050000s1')
Traceback (most recent call last):
  File "/usr/local/share/sage-4.8/local/bin/sage-list-packages", line 21, in <module>
    os.makedirs(SAGE_ROOT_TMP)
  File "/usr/local/share/sage-4.8/local/lib/python/os.py", line 157, in makedirs
    mkdir(name, mode)
OSError: [Errno 13] Permission denied: '/usr/local/share/sage-4.8/tmp'
Using SAGE Server http://www.sagemath.org//packages

Optional package list (shown above) appears to be currently not available or corrupted (network error?).
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/home/wstein/<ipython console> in <module>()

/usr/local/share/sage-4.8/local/lib/python2.6/site-packages/sage/schemes/elliptic_curves/constructor.pyc in EllipticCurve(x, y, j)
    293         
    294     if isinstance(x, str):
--> 295         return ell_rational_field.EllipticCurve_rational_field(x)
    296         
    297     if rings.is_RingElement(x) and y is None:

/usr/local/share/sage-4.8/local/lib/python2.6/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in __init__(self, ainvs, extra)
    197             label = ainvs
    198             X = sage.databases.cremona.CremonaDatabase()[label]
--> 199             EllipticCurve_number_field.__init__(self, Q, list(X.a_invariants()))
    200             self.__np = {}
    201             self.__gens = {}

AttributeError: 'NoneType' object has no attribute 'a_invariants'

Apply patch at #12341

Apply attachment:trac_12565-rebase.patch

Attachments (2)

trac12565.patch (3.8 KB) - added by R. Andrew Ohana 11 years ago.
trac_12565-rebase.patch (5.1 KB) - added by John Cremona 10 years ago.
Rebasing after #12341's rebase

Download all attachments as: .zip

Change History (28)

comment:1 Changed 11 years ago by John Cremona

That is really stupid. I'm sure it never used to do that --but why is there not a dictest for this construction on a failing string? I wonder if this came in with the new database implmentation. The error should surely be raised in the call to teh class contructor with the string, not just returning None.

comment:2 in reply to:  1 Changed 11 years ago by R. Andrew Ohana

Replying to cremona:

I wonder if this came in with the new database implementation.

Yup, with 4.7.2 (pre database change)

sage: EllipticCurve('10a1')
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)

/home/ohanar/sage-dev/sage-4.7.2-sage.math.washington.edu-x86_64-Linux/<ipython console> in <module>()

/home/ohanar/sage-dev/sage-4.7.2-sage.math.washington.edu-x86_64-Linux/local/lib/python2.6/site-packages/sage/schemes/elliptic_curves/constructor.pyc in EllipticCurve(x, y, j)
    293         
    294     if isinstance(x, str):
--> 295         return ell_rational_field.EllipticCurve_rational_field(x)
    296         
    297     if rings.is_RingElement(x) and y is None:

/home/ohanar/sage-dev/sage-4.7.2-sage.math.washington.edu-x86_64-Linux/local/lib/python2.6/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in __init__(self, ainvs, extra)
    196         if isinstance(ainvs, str):
    197             label = ainvs
--> 198             X = sage.databases.cremona.CremonaDatabase()[label]
    199             EllipticCurve_number_field.__init__(self, Q, list(X.a_invariants()))
    200             self.__np = {}

/home/ohanar/sage-dev/sage-4.7.2-sage.math.washington.edu-x86_64-Linux/local/lib/python2.6/site-packages/sage/databases/cremona.pyc in __getitem__(self, N)
    453                 return sage.databases.db.Database.__getitem__(self, N)
    454             else:
--> 455                 return self.elliptic_curve(N)
    456         try:
    457             N = int(N)

/home/ohanar/sage-dev/sage-4.7.2-sage.math.washington.edu-x86_64-Linux/local/lib/python2.6/site-packages/sage/databases/cremona.pyc in elliptic_curve(self, label)
    693             else:
    694                 message = "There is no elliptic curve with label "+label+" in the currently available databases"
--> 695             raise RuntimeError, message
    696         
    697         F = elliptic.EllipticCurve(e[0])

RuntimeError: There is no elliptic curve with label 10a1 in the database (note: use lower case letters!)

I'll go see about fixing this now.

For the other bug, that looks like it might not be specific to the cremona database.

comment:3 Changed 11 years ago by R. Andrew Ohana

ok, that first bug was really stupid. The error message was being created, but it was never being raised. This also gives the correct error for the other end of the spectrum, but it doesn't fix the scary permissions error. I'm going to see what that is about now.

comment:4 Changed 11 years ago by R. Andrew Ohana

Authors: R. Andrew Ohana
Status: newneeds_review

so #12341 fixes the other issue, please review these two tickets

comment:5 Changed 11 years ago by William Stein

Status: needs_reviewneeds_work

I think this error message is annoying:

RuntimeError: There is no elliptic curve with label 10a1 in the database (note: use lower case letters!) 

I can see putting " (note: use lower case letters!) " if the user input "10A1", but if they already input lower case letters, why add that remark!?

comment:6 Changed 11 years ago by R. Andrew Ohana

Dependencies: #11341
Status: needs_workneeds_review

ok, changed that by making it accept old cremona labels (and having the label parser actually throw errors if there is a problem with the label)

comment:7 Changed 11 years ago by R. Andrew Ohana

dependency was added since otherwise this patch no longer applies cleanly

comment:8 Changed 11 years ago by William Stein

Status: needs_reviewneeds_work

Include an example that illustrates the following: "accept old cremona labels"

comment:9 in reply to:  8 Changed 11 years ago by R. Andrew Ohana

Replying to was:

Include an example that illustrates the following: "accept old cremona labels"

Where? In the elliptic curve constructor, the database method, or both?

comment:10 Changed 11 years ago by William Stein

In the elliptic curve constructor, the database method, or both?

In both, and use different examples in each case (it's your chance to be creative!)

Changed 11 years ago by R. Andrew Ohana

Attachment: trac12565.patch added

comment:11 Changed 11 years ago by R. Andrew Ohana

Status: needs_workneeds_review

ok, hopefully there that satisfies all of your complaints :)

comment:12 Changed 11 years ago by R. Andrew Ohana

Dependencies: #11341#12341

oops, put the wrong ticket as the dependency

comment:13 Changed 11 years ago by R. Andrew Ohana

I found the root of the permissions issue that #12341 avoids and have made the ticket #12578 for it.

comment:14 Changed 11 years ago by John Cremona

Reviewers: John Cremona
Status: needs_reviewneeds_work

I get an error applying the patch to 5.0.beta5, at line 677 in sage/databases/cremona.py with reject messsage

Hunk #4 FAILED at 677
1 out of 5 hunks FAILED -- saving rejects to file sage/databases/cremona.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac12565.patch

but I'm not sure what is causing this. the patch looks fine to me. After app;lying the patch (without that hunk) I still get

sage: EllipticCurve('10a1')
...
RuntimeError: There is no elliptic curve with label 10a1 in the database (note: use lower case letters!)

If you can fix this I'm sure I'll be able to give a positive review quickly.

comment:15 in reply to:  14 ; Changed 11 years ago by R. Andrew Ohana

Status: needs_workneeds_review

Replying to cremona:

I get an error applying the patch to 5.0.beta5, at line 677 in sage/databases/cremona.py with reject messsage

This patch was based off of #12341 which also touches this file, which is why I added it as a dependency. (The reason why it fails is due to the line following the one that generates the error message, it is changed in #12341)

comment:16 Changed 11 years ago by R. Andrew Ohana

Description: modified (diff)

comment:17 in reply to:  15 Changed 11 years ago by John Cremona

Replying to ohanar:

Replying to cremona:

I get an error applying the patch to 5.0.beta5, at line 677 in sage/databases/cremona.py with reject messsage

This patch was based off of #12341 which also touches this file, which is why I added it as a dependency. (The reason why it fails is due to the line following the one that generates the error message, it is changed in #12341)

Sorry, my mistake -- I did not notice the dependency. I will get back to it.

comment:18 Changed 11 years ago by John Cremona

Status: needs_reviewpositive_review

With #12341 this applies (to 5.0.beta5) and works fine.

comment:19 Changed 11 years ago by Jeroen Demeyer

Milestone: sage-5.0sage-pending

comment:20 Changed 10 years ago by John Cremona

Status: positive_reviewneeds_work
Work issues: needs rebase

Changed 10 years ago by John Cremona

Attachment: trac_12565-rebase.patch added

Rebasing after #12341's rebase

comment:21 Changed 10 years ago by John Cremona

Description: modified (diff)
Status: needs_workneeds_review
Work issues: needs rebase

I have rebased the patch so that it now works after the rebased #12341.

This was not quite trivial as a little extra coding was needed so that "Old Cremona labels" could be parsed properly using the regexp method.

I tested sage/databses/cremona.py and all in sage/schemes/elliptic_curves.

comment:22 Changed 10 years ago by R. Andrew Ohana

The regexp code looks good to me.

comment:23 Changed 10 years ago by John Cremona

Status: needs_reviewpositive_review

comment:24 Changed 10 years ago by John Cremona

To confirm: I checked that the rebased patch applies to 5.4.rc1+trac_12341.patch (after that was rebased), and that all tests in schemes/elliptic_curves pass (1) without database_ell_cremona installed, (2) with it installed, without "-optional database_cremona_ellcurve", (3) with it installed and with "-optional database_cremona_ellcurve".

The only failed test is the one in heegner.py which fails when the optional database is installed for reasons spelled out elsewhere and to be fixed in #13547.

comment:25 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-pendingsage-5.5

comment:26 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.5.beta2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.