Opened 12 years ago
Closed 12 years ago
#4822 closed enhancement (fixed)
[with patch, positive review] Tweak to the error message for EllipticCurve
Reported by: | ljpk | Owned by: | davidloeffler |
---|---|---|---|
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: | 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 12 years ago by
- Type changed from defect to enhancement
comment:2 Changed 12 years ago by
- Component changed from number theory to elliptic curves
- Owner changed from was to davidloeffler
Changed 12 years ago by
comment:3 Changed 12 years ago by
- Summary changed from Tweak to the error message for EllipticCurve to [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 12 years ago by
- Summary changed from [with patch, needs review] Tweak to the error message for EllipticCurve to [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 12 years ago by
- Summary changed from [with patch, under review] Tweak to the error message for EllipticCurve to [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 12 years ago by
- Merged in set to Sage 4.1.2.alpha0
- Resolution set to fixed
- Reviewers set to Chris Wuthrich
- Status changed from new to closed
Merged both patches.
Applies to 4.1.1