#6887: Implement elliptic curve isogenies (continued)
-------------------------------+--------------------------------------------
Reporter: cremona | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone:
Component: elliptic curves | Keywords: elliptic curve isogeny
Work_issues: | Author:
Upstream: N/A | Reviewer:
Merged: |
-------------------------------+--------------------------------------------
Comment(by wuthrich):
Sorry for being so slow in reviewing this. This is a very good and long
patch, so I will allow myself a bit more time to look at it. So far I only
spotted a few minor issues.
* Coverage issue which should be easy to fix :
{{{
ell_curve_isogeny.py
SCORE ell_curve_isogeny.py: 98% (79 of 80)
Missing doctests:
* _isogeny_machine(Ew, f, ker, a, iso=None, E=None):
Possibly wrong (function name doesn't occur in doctests):
* unfill_isogeny_matrix(M):
}}}
* The patch applies fine and the first few tests I did pass. But when you
deleted the top part of the patch you (=John) probably deleted a little
bit too much, the presentation here is missing something in the beginning.
I don't think that it harms the patch though.
* Naming : Do you want to call it l_isogenies ? I agree that
prime_degree_isogenies is a bit long, but would have the advantage of
being clear about what l can be and about the possible confusion with 1
and I (depending on the font). Or isogenies_of_prime_degree that would
make the function appear in .isog<tab>.
* In line 1266 of ell_number_field.py, docstring of is_isogenous, you
write
''If ``True``, this test should be followed by a rigorous test (not
fully implemented).''
What do you mean by should ? Do you mean "this test is followed by a
rigourous test if it is implemented for the given curve..." ? I think I
know what goes on from the second to last example, but maybe it would be
good to have a sentence or two about it.
Of course, these are very minor things. once I have played around a little
with the code and read a bit more of it, I will give a positive review....
Chris.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/6887#comment:8>
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.
