Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
williamstein opened this issue Feb 15, 2012 · 7 comments
Closed

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

williamstein opened this issue Feb 15, 2012 · 7 comments

Comments

@williamstein
Copy link
Contributor

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.

Component: elliptic curves

Author: William Stein

Reviewer: Robert Bradshaw

Merged: sage-5.0.beta5

Issue created by migration from https://trac.sagemath.org/ticket/12517

@williamstein
Copy link
Contributor Author

Attachment: trac_12715.patch.gz

@williamstein
Copy link
Contributor Author

Author: William Stein

@JohnCremona
Copy link
Member

comment:3

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!

@williamstein
Copy link
Contributor Author

comment:4

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

@JohnCremona
Copy link
Member

comment:5

Replying to @williamstein:

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.

@jdemeyer
Copy link

Reviewer: Robert Bradshaw

@jdemeyer
Copy link

Merged: sage-5.0.beta5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants