Opened 3 years ago
Closed 22 months ago
#12565 closed defect (fixed)
E = EllipticCurve('10a1') gives a stupid traceback (rather than a smart one)
Reported by: | was | Owned by: | 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: |
Description (last modified by 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
Attachments (2)
Change History (28)
comment:1 follow-up: ↓ 2 Changed 3 years ago by cremona
comment:2 in reply to: ↑ 1 Changed 3 years ago by ohanar
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 3 years ago by ohanar
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 3 years ago by ohanar
- Status changed from new to needs_review
so #12341 fixes the other issue, please review these two tickets
comment:5 Changed 3 years ago by was
- Status changed from needs_review to needs_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 3 years ago by ohanar
- Dependencies set to #11341
- Status changed from needs_work to needs_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 3 years ago by ohanar
dependency was added since otherwise this patch no longer applies cleanly
comment:8 follow-up: ↓ 9 Changed 3 years ago by was
- Status changed from needs_review to needs_work
Include an example that illustrates the following: "accept old cremona labels"
comment:9 in reply to: ↑ 8 Changed 3 years ago by ohanar
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 3 years ago by was
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 3 years ago by ohanar
comment:11 Changed 3 years ago by ohanar
- Status changed from needs_work to needs_review
ok, hopefully there that satisfies all of your complaints :)
comment:12 Changed 3 years ago by ohanar
- Dependencies changed from #11341 to #12341
oops, put the wrong ticket as the dependency
comment:13 Changed 3 years ago by ohanar
comment:14 follow-up: ↓ 15 Changed 3 years ago by cremona
- Reviewers set to John Cremona
- Status changed from needs_review to needs_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 ; follow-up: ↓ 17 Changed 3 years ago by ohanar
- Status changed from needs_work to needs_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 3 years ago by ohanar
- Description modified (diff)
comment:17 in reply to: ↑ 15 Changed 3 years ago by 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 3 years ago by cremona
- Status changed from needs_review to positive_review
With #12341 this applies (to 5.0.beta5) and works fine.
comment:19 Changed 2 years ago by jdemeyer
- Milestone changed from sage-5.0 to sage-pending
comment:20 Changed 23 months ago by cremona
- Status changed from positive_review to needs_work
- Work issues set to needs rebase
comment:21 Changed 23 months ago by cremona
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues needs rebase deleted
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 23 months ago by ohanar
The regexp code looks good to me.
comment:23 Changed 23 months ago by cremona
- Status changed from needs_review to positive_review
comment:24 Changed 23 months ago by 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 22 months ago by jdemeyer
- Milestone changed from sage-pending to sage-5.5
comment:26 Changed 22 months ago by jdemeyer
- Merged in set to sage-5.5.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
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.