#5976: [with new patch; with review] Add an Elliptic Curve Isogeny object
---------------------------+------------------------------------------------
 Reporter:  shumow         |       Owner:  shumow         
     Type:  enhancement    |      Status:  assigned       
 Priority:  major          |   Milestone:  sage-4.0.1     
Component:  number theory  |    Keywords:  Elliptic Curves
---------------------------+------------------------------------------------

Comment(by cremona):

 I don't have time at the moment to go into this in a lot of detail.  The
 patch applies fine;  the examples I tried (including a 37-isogeny over Q)
 worked perfectly.

 I have a few minor comments listed below.  I found a lot of minor glitches
 in the docstrings when building the reference manual, and have fixed
 these.  My patch was supposed to be applied after yours, but owing to a
 mistake on my part is is a self-contained patch including all your code.
 Sorry about that, I know that it makes it harder to see what I changed --
 docstrings only!

 It might be a good idea if David K also looked at it, since he is more
 familiar with "his" algorithm than I am;  I also know nothing about
 Stark's algorithm.

 Overall this is a very impressive piece of work (over 4000 lines!) and I
 think it should go in as soon as possible, since it is only when people
 start to use it for real that problems (if there are any) will surface.

 Now for the (mainly) minor points:

     * Several ReST glitches (e.g. missing double ::). [fixed]

     * In the doctests for the helper functions I think it would be better
 to
       have direct rather than indirect doctests.  For example in the tests
 at
       lines 98-99 surely the fact that the twosides agree is a tautology
       since the same code is used on both sides?  And in
       compute_codomain_formula(), why not put in a direct doctest.  (In
 that
       function you do not really need A1,A2,A3 and you can define
 a1,...,a6
       in one line via a1,a2,a3,a4,a6=E.ainvs().  Similarly elsewhere.)

     * compute_vw_kohel_even_deg1() does not use parameter a6.

     * l.225: incomplete sentence! [look for "missing text" in the source,
 I put in at least 2]

     * I like the very clear list of private data for the isogeny class,
       where all are set to None by default, though I suspect this is more
       like C++ than Python.  It makes it *very* much easier to see what is
       going on.

     * There are lots of good examples.

     * The docstring for the isogeny method for elliptic curves should say
       more: it should certainly detail the input parameters and give more
       examples (possibly copied from the EllipticCurveIsogeny
       documentation).  The point is the when someone E.<tab> and sees
       E.isogeny is available and then types E.isogeny? they need to see
 how
       to use that function.

-- 
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/5976#comment:31>
Sage <http://sagemath.org/>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica, 
and MATLAB

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"sage-trac" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to 
[email protected]
For more options, visit this group at 
http://groups.google.com/group/sage-trac?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply via email to