Ticket #12517 (closed enhancement: fixed)

Opened 15 months ago

Last modified 15 months ago

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

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

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

trac_12715.patch Download (1.5 KB) - added by was 15 months ago.

Change History

Changed 15 months ago by was

comment:1 Changed 15 months ago by was

  • Status changed from new to needs_review
  • Authors set to William Stein

comment:2 Changed 15 months ago by robertwb

  • Status changed from needs_review to positive_review

comment:3 Changed 15 months ago by 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 follow-up: ↓ 5 Changed 15 months ago by was

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 15 months ago by 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 15 months ago by jdemeyer

  • Reviewers set to Robert Bradshaw

comment:7 Changed 15 months ago by jdemeyer

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sage-5.0.beta5
Note: See TracTickets for help on using tickets.