Opened 13 years ago

Closed 12 years ago

Last modified 5 years ago

#4264 closed enhancement (fixed)

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

Reported by: was 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 12 years ago.
exported against 4.2.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 13 years ago by 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 13 years ago by cremona

  • Priority changed from major to minor

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 12 years ago by davidloeffler

  • Component changed from number theory to elliptic curves
  • Owner changed from was to davidloeffler

comment:4 Changed 12 years ago by cremona

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

comment:5 Changed 12 years ago by davidloeffler

  • Owner changed from davidloeffler to (none)

comment:6 follow-up: Changed 12 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 12 years ago by 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 12 years ago by wuthrich

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

Changed 12 years ago by wuthrich

exported against 4.2.

comment:9 Changed 12 years ago by wuthrich

  • Status changed from new to needs_review

I hope I did not miss any a_invs or a_invariants.

comment:10 Changed 12 years ago by mhansen

  • Authors set to Christian Wuthrich
  • Reviewers set to Mike Hansen
  • Status changed from needs_review to positive_review

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

comment:11 Changed 12 years ago by mhansen

  • Merged in set to sage-4.2.1.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 5 years ago by chapoton

  • Authors changed from Christian Wuthrich to Chris Wuthrich
  • Report Upstream set to N/A
Note: See TracTickets for help on using tickets.