change E.a_invariants() for an elliptic curve to return a tuple

[williamstein]

For consistency with b_invariants, etc., and to emphasize immutability, it would be
good for E.a_invariants() to return a tuple. Changing this could change lots of doctests, etc., so this isn't trivial.

See trac #4262 for a related ticket

Component: elliptic curves

Author: Chris Wuthrich

Reviewer: Mike Hansen

Merged: sage-4.2.1.alpha0

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

[JohnCremona]

comment:1

Quick comment: the cached self.__ainvs should actually be a tuple. So the only code to change is in line 142 in ell_generic.py, from this

        self.__ainvs = ainvs

to this

        self.__aincs = tuple(ainvs)

as well as the doctests.

[JohnCremona]

comment:2

I have made a patch (not yet attached) which implements this. It was easy to do what was suggested (and make the consequent cosmetic changes in doctests from [...] to (...) ) but there were two similar but distinct other issues:

• Several Sage functions (the __init__ function in the EllipticCurve classes) expect the a-invs input parameters to be a list and not a tuple. I tried changing them to accept tuples but it caused too many difficulites with parsing different input possibilities so I reverted that.
• In several places where elliptic curves in other systems are initialised (e.g. mwrank, gp) lists are required for the parsing done by the wrappers.

In all the above I sorted everything out by inserting list(...) around blah.ainvs() or blah.a_invariants(), which works but is ugly. Is there a better way? Even just having a new function a_list() to be list(self.ainvs()) would be a bit cleaner. We already have the unnecessary synonyms a_invariants() and ainvs().

I'll wait for reaction before going further. In particular, I have not yet tested anything outside the elliptic_curves directory, e.g. the tutorial.

[JohnCremona]

comment:4

Doesn't 9 months go quickly? I thought this had been fixed long ago. No time now though...

[chriswuthrich]

comment:6

I think we won't need a a_list. I'd prefer having list() everywhere, even if it is ugly.

Could you post your first draft of a patch here ? I will try to work on it.

Chris.

[JohnCremona]

comment:7

Replying to @categorie:

I think we won't need a a_list. I'd prefer having list() everywhere, even if it is ugly.

Could you post your first draft of a patch here ? I will try to work on it.

Chris.

Sorry, but after a year I am sure that it is lost for ever. I should have uploaded it anyway with a "needs more work" tag. Anyway, after a year of version changes it would never have merged without a lot of work.

[chriswuthrich]

comment:8

That is alright. If I get to do it, I will start from scratch, then.

[chriswuthrich]

exported against 4.2.

[chriswuthrich]

comment:9

Attachment: trac_4264.patch.gz

I hope I did not miss any a_invs or a_invariants.

[mwhansen]

comment:10

Looks good to me. Passes all tests with -long.

[mwhansen]

Author: Christian Wuthrich

[mwhansen]

Reviewer: Mike Hansen

[mwhansen]

Merged: sage-4.2.1.alpha0

[fchapoton]

Changed author from Christian Wuthrich to Chris Wuthrich