Opened 14 years ago

Closed 13 years ago

Last modified 6 years ago

#4264 closed enhancement (fixed)

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

Reported by: William Stein Owned by:
Priority: minor Milestone: sage-4.2.1
Component: elliptic curves Keywords:
Cc: Merged in: sage-4.2.1.alpha0
Authors: Chris Wuthrich Reviewers: Mike Hansen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

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

Attachments (1)

trac_4264.patch (22.8 KB) - added by wuthrich 13 years ago.
exported against 4.2.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 14 years ago by John Cremona

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.

comment:2 Changed 14 years ago by John Cremona

Priority: majorminor

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.

comment:3 Changed 13 years ago by David Loeffler

Component: number theoryelliptic curves
Owner: changed from William Stein to David Loeffler

comment:4 Changed 13 years ago by John Cremona

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

comment:5 Changed 13 years ago by David Loeffler

Owner: David Loeffler deleted

comment:6 Changed 13 years ago by wuthrich

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.

comment:7 in reply to:  6 Changed 13 years ago by John Cremona

Replying to wuthrich:

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.

comment:8 Changed 13 years ago by wuthrich

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

Changed 13 years ago by wuthrich

Attachment: trac_4264.patch added

exported against 4.2.

comment:9 Changed 13 years ago by wuthrich

Status: newneeds_review

I hope I did not miss any a_invs or a_invariants.

comment:10 Changed 13 years ago by Mike Hansen

Authors: Christian Wuthrich
Reviewers: Mike Hansen
Status: needs_reviewpositive_review

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

comment:11 Changed 13 years ago by Mike Hansen

Merged in: sage-4.2.1.alpha0
Resolution: fixed
Status: positive_reviewclosed

comment:12 Changed 6 years ago by Frédéric Chapoton

Authors: Christian WuthrichChris Wuthrich
Report Upstream: N/A
Note: See TracTickets for help on using tickets.