#6583: Implement 2-isogeny descent over QQ natively in Sage using ratpoints
-------------------------------+--------------------------------------------
Reporter: rlm | Owner: rlm
Type: enhancement | Status: positive_review
Priority: major | Milestone: sage-4.3
Component: elliptic curves | Keywords:
Work_issues: | Author: Robert Miller
Upstream: N/A | Reviewer:
Merged: |
-------------------------------+--------------------------------------------
Changes (by was):
* status: needs_review => positive_review
Comment:
REPORT:
1. I can't apply the patch to 4.3.alpha1:
{{{
patching file sage/libs/flint/zmod_poly.pxd
Hunk #2 FAILED at 249
1 out of 2 hunks FAILED -- saving rejects to file
sage/libs/flint/zmod_poly.pxd.rej
abort: patch failed to apply
}}}
The rebase is trivial, and I've posted it.
---
2. The main concern in Chris's report is just that this code doesn't yet
do enough. However, rlm isn't just writing this as some one-off thing for
a little project. He's doing his Ph.D. thesis mostly on descent (and its
applications), and this is what he'll be working on fulltime, probably for
the next year (for his thesis, then his postdoc). So I think getting this
in now makes *perfect* sense, instead of waiting until we get a big patch
bomb later.
3. All those cdef'd methods with no doctests and minimal documentation
isn't good, just as Chris says. You could open another ticket and fix
that, since it will make it way easier for others to work on (and use in
surprising ways!) this code if those functions are documented and tested.
For each cdef'd method, just make a corresponding def'd method called
"test_..." that calls the cdef'd method, then put the tests there. You
already do just that in *some* cases, e.g., {{{def
test_els(a,b,c,d,e):}}}.
4. Docstrings like this would be more readable if they used latex:
{{{
Given an elliptic curve E with a two-isogeny phi : E --> E' and dual
isogeny
799 phi', runs a two-isogeny descent on E, returning n1,
n2, n1' and n2'. Here
800 n1 is the number of quartic covers found with a
rational point, and n2 is
801 the number which are ELS.
}}}
5. All tests pass with this code applied to sage-4.3.alpha1.
In summary:
I've applied the patches, skimmed them, and run the test suite. Positive
review.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6583#comment:19>
Sage <http://www.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.
