New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
E = EllipticCurve('10a1') gives a stupid traceback (rather than a smart one) #12565
Comments
comment:1
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
Replying to @JohnCremona:
Yup, with 4.7.2 (pre database change)
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
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. |
Author: R. Andrew Ohana |
comment:4
so #12341 fixes the other issue, please review these two tickets |
comment:5
I think this error message is annoying:
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
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) |
Dependencies: #11341 |
comment:7
dependency was added since otherwise this patch no longer applies cleanly |
comment:8
Include an example that illustrates the following: "accept old cremona labels" |
comment:9
Replying to @williamstein:
Where? In the elliptic curve constructor, the database method, or both? |
comment:10
In both, and use different examples in each case (it's your chance to be creative!) |
Attachment: trac12565.patch.gz |
comment:11
ok, hopefully there that satisfies all of your complaints :) |
comment:12
oops, put the wrong ticket as the dependency |
comment:15
Replying to @JohnCremona:
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) |
This comment has been minimized.
This comment has been minimized.
comment:17
Replying to @ohanar:
Sorry, my mistake -- I did not notice the dependency. I will get back to it. |
comment:18
With #12341 this applies (to 5.0.beta5) and works fine. |
Work Issues: needs rebase |
Rebasing after #12341's rebase |
comment:21
Attachment: trac_12565-rebase.patch.gz 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. |
This comment has been minimized.
This comment has been minimized.
Changed work issues from needs rebase to none |
comment:22
The regexp code looks good to me. |
comment:24
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. |
Merged: sage-5.5.beta2 |
I don't like this error:
I would expect something like:
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:
Apply patch at #12341
Apply attachment: trac_12565-rebase.patch
Depends on #12341
Component: elliptic curves
Author: R. Andrew Ohana
Reviewer: John Cremona
Merged: sage-5.5.beta2
Issue created by migration from https://trac.sagemath.org/ticket/12565
The text was updated successfully, but these errors were encountered: