Opened 14 years ago
Closed 13 years ago
#4822 closed enhancement (fixed)
[with patch, positive review] Tweak to the error message for EllipticCurve
Reported by: | L J P Kilford | Owned by: | David Loeffler |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.1.2 |
Component: | elliptic curves | Keywords: | |
Cc: | Merged in: | Sage 4.1.2.alpha0 | |
Authors: | John Cremona | Reviewers: | Chris Wuthrich |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
I was using SAGE with the small version of the CremonaDatabase?, and tried the following, which does not work because the conductor is too high:
EllipticCurve("10001a1")
I think it would be useful if the error message not only said "this curve is not in the database" (which is indeed true) but also checked to see if one was using the small database of curves, and if so told the user how to access the larger version using the incantation
!sage -i database_cremona_ellcurve-2005.11.03
or otherwise.
Attachments (2)
Change History (8)
comment:1 Changed 14 years ago by
Type: | defect → enhancement |
---|
comment:2 Changed 13 years ago by
Component: | number theory → elliptic curves |
---|---|
Owner: | changed from William Stein to David Loeffler |
Changed 13 years ago by
Attachment: | trac_4822-ecdb_error_msg.patch added |
---|
comment:3 Changed 13 years ago by
Summary: | Tweak to the error message for EllipticCurve → [with patch, needs review] Tweak to the error message for EllipticCurve |
---|
The patch deals with this in a more intelligent way. I did not add doctests since it's hard to do that when the output message depends on whether or not the extra database is installed. But I did test it before and after installing the optional database. The message suggests installing the optional database iff the desired conductor is betweem 10000 and 13000 and the optional db is not already installed; I did not actually include the command-line to install it though.
The second patch fixes a doctest which otherwise fails when run after installing the optional database (but is otherwise independent of this ticket, except that I wrote it so was my fault anyway!)
comment:4 Changed 13 years ago by
Summary: | [with patch, needs review] Tweak to the error message for EllipticCurve → [with patch, under review] Tweak to the error message for EllipticCurve |
---|
The patch is ok (so far only tested without the optional package). I will test it with it later today.
Maybe while we are at it I could suggest that the same change is done to cremona_label()
.
This is without the optional package:
sage: E= EllipticCurve([3,10]) sage: E.cremona_label() --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) /home/chrigu/.sage/temp/linux_ljo8/12909/_home_chrigu__sage_init_sage_0.py in <module>() /usr/local/sage/local/lib/python2.6/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in cremona_label(self, space) 3305 X = self.database_curve() 3306 except RuntimeError: -> 3307 raise RuntimeError, "Cremona label not known for %s."%self 3308 self.__cremona_label = X.__cremona_label 3309 return self.cremona_label(space) RuntimeError: Cremona label not known for Elliptic Curve defined by y^2 = x^3 + 3*x + 10 over Rational Field. sage: E.conductor() 44928
Chris.
ps : 'I don't know if there is an official 'under review'.
comment:5 Changed 13 years ago by
Summary: | [with patch, under review] Tweak to the error message for EllipticCurve → [with patch, positive review] Tweak to the error message for EllipticCurve |
---|
This patch also works with the optional package. I give this ticket a positive review and I agree that the command-line to install the optional package should better not be included. It is very clear what to do as it is now.
The second [independent] patch obtains also a positive review with this.
I propose to address the issue that I raised on
cremona_label()
in a new ticket.
Chris.
comment:6 Changed 13 years ago by
Authors: | → John Cremona |
---|---|
Merged in: | → Sage 4.1.2.alpha0 |
Resolution: | → fixed |
Reviewers: | → Chris Wuthrich |
Status: | new → closed |
Merged both patches.
Applies to 4.1.1