Opened 11 years ago

Closed 11 years ago

#12517 closed enhancement (fixed)

EllipticCurve(E.a_invariants()) doesn't work

Reported by: William Stein Owned by: John Cremona
Priority: minor Milestone: sage-5.0
Component: elliptic curves Keywords:
Cc: Merged in: sage-5.0.beta5
Authors: William Stein Reviewers: Robert Bradshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

At some point E.a_invariants() was changed to return a 5-tuple instead of a list of 5 numbers. I have no idea why. But this means that:

sage: E = EllipticCurve([1..5])
sage: EllipticCurve(E.a_invariants())
TypeError: invalid input to EllipticCurve constructor

Ugh. Fix this, i.e., make the constructor take a tuple or a list.

Attachments (1)

trac_12715.patch (1.5 KB) - added by William Stein 11 years ago.

Download all attachments as: .zip

Change History (8)

Changed 11 years ago by William Stein

Attachment: trac_12715.patch added

comment:1 Changed 11 years ago by William Stein

Authors: William Stein
Status: newneeds_review

comment:2 Changed 11 years ago by Robert Bradshaw

Status: needs_reviewpositive_review

comment:3 Changed 11 years ago by John Cremona

See #4262 for the origin of this change, after a report by was (!) and #4264 by the patch which implemented it written mosstly by me. As it says at that ticket, I did (of course) try to get the EllipticCurve? constructor to accept tuples, but for some reason at the time I failed to get it to work. So either you people are cleverer now than you or I were then, or other changes have made that easier.

Of course I approve of the change!

comment:4 Changed 11 years ago by William Stein

All my patch does is change an "isinstance(., list)" to "isinstance(., (list, tuple))" in one line. That's it. You worry me.

comment:5 in reply to:  4 Changed 11 years ago by John Cremona

Replying to was:

All my patch does is change an "isinstance(., list)" to "isinstance(., (list, tuple))" in one line. That's it. You worry me.

Don't worry, it was over 3 years ago and needed masses of doctest output changes which is the main thing I remember doing. Looking at my old comments in #4264 it looks as if I was trying to change {{{init}} functions in classes instead of what you did.

comment:6 Changed 11 years ago by Jeroen Demeyer

Reviewers: Robert Bradshaw

comment:7 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta5
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.