Opened 14 years ago

Closed 14 years ago

#5346 closed defect (fixed)

[with patch, positive review] Some doctests in schemes/elliptic_curves/ell_rational_field.py fail with optional database installed

Reported by: Michael Abshoff Owned by: William Stein
Priority: major Milestone: sage-3.4.2
Component: number theory Keywords:
Cc: John Cremona Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Reported by Jan Groenewald in http://groups.google.com/group/sage-devel/browse_thread/thread/d5db5c25fbce1e99

sage -t  "devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py"
**********************************************************************
File "/usr/local/src/sage-3.3/devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py", line 2675:
    sage: E.cremona_label()
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: Cremona label not known for Elliptic Curve defined by y^2 + x*y + 3*y = x^3 + 2*x^2 + 4*x + 5 over Rational Field.
Got:
    '10351a1'
**********************************************************************

Attachments (1)

trac_5346.patch (1.6 KB) - added by John Cremona 14 years ago.
Apply to 3.4.1

Download all attachments as: .zip

Change History (7)

comment:1 Changed 14 years ago by Michael Abshoff

This problem is ironically introduced by having database_cremona_ellcurve-20071019.spkg installed :)

Cheers,

Michael

comment:2 Changed 14 years ago by Georg S. Weber

For the record:

The optional database covers conductor ranges from 10000 to 130000 AFAIK. So an obviously working patch (to be tested ...) already discussed elsewhere (I don't remember exactly who had this idea first, it wasn't me) would be to exchange the curve with conductor 10351 in the doctest, with a curve having conductor bigger than 130000.

Cheers, gsw

comment:3 in reply to:  2 Changed 14 years ago by John Cremona

Replying to GeorgSWeber:

For the record:

The optional database covers conductor ranges from 10000 to 130000 AFAIK. So an obviously working patch (to be tested ...) already discussed elsewhere (I don't remember exactly who had this idea first, it wasn't me) would be to exchange the curve with conductor 10351 in the doctest, with a curve having conductor bigger than 130000.

I agree, and suggest

sage: E = EllipticCurve([1, -1, 0, -79, 289]) 
sage: E.cremona_label()
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)

/home/john/.sage/temp/ubuntu/1126/_home_john__sage_init_sage_0.py in <module>()

/home/john/sage-3.4/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/ell_rational_field.pyc in cremona_label(self, space)
   3034                 X = self.database_curve()
   3035             except RuntimeError:
-> 3036                 raise RuntimeError, "Cremona label not known for %s."%self
   3037             self.__cremona_label = X.__cremona_label
   3038             return self.cremona_label(space)

RuntimeError: Cremona label not known for Elliptic Curve defined by y^2 + x*y = x^3 - x^2 - 79*x + 289 over Rational Field.

as that curve will one day have label 234446a1 (or b1 or c1, I forget).

Cheers, gsw

Changed 14 years ago by John Cremona

Attachment: trac_5346.patch added

Apply to 3.4.1

comment:4 Changed 14 years ago by John Cremona

Summary: Sage 3.3: schemes/elliptic_curves/ell_rational_field.py fails to doctest with optional database installed[with patch, needs review] Some doctests in schemes/elliptic_curves/ell_rational_field.py fail with optional database installed

The patch cahnges taht example to the one with condictor 234446 as I suggested.

It also make another doctest work ok with & without the database (at line 4941) by hard-wiring in some points instead of getting the gens.

I tested this on sage-3.4.1 with & without the database installed.

comment:5 Changed 14 years ago by Michael Abshoff

Summary: [with patch, needs review] Some doctests in schemes/elliptic_curves/ell_rational_field.py fail with optional database installed[with patch, positive review] Some doctests in schemes/elliptic_curves/ell_rational_field.py fail with optional database installed

Thanks for taking care of this John. Doctests pass, pass reads good and fixes the problem. Positve review.

Cheers,

Michael

comment:6 Changed 14 years ago by Michael Abshoff

Milestone: sage-4.0sage-3.4.2
Resolution: fixed
Status: newclosed

Merged in Sage 3.4.2.alpha0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.