Skimming the file:
1. Improve this comment you introduce in sage/schemes/elliptic_curves/all.py
{{{
# Temp until we integrate this more fully:
}}}
2. This is a run on sentence:
{{{
The following examples are from trac ticket #10152, previously Magma was finding more integral points on these curves than Sage.
}}}
3. This stuff seems weird in the file {{{ell_int_points.py}}}:
{{{
70 # File: intpts.sage
71 # Created: Thu Jul 01 04:22 PM 2010 C
72 # Last Change: Fri Jul 02 12:51 PM 2010
73 # Original Magma Code: Thotsaphon "Nook" Thongjunthug
74 # Sage version: Radoslav Kirov, Jackie Anderson
}}}
I think instead you should just add another line to the AUTHORS block that explains that this file is based on work of Thotsaphon "Nook" Thongjunthug that was initially ported to Sage by Radoslav Kirov and Jackie Anderson.
4. Since you're adding a whole new file that I wrote none of, using this in the copyright file is weird:
{{{
# Copyright (C) 2005,2006,2007 William Stein
}}}
You could assign the copyright to me (I'm OK with that). But make the date 2011. Or keep the copyright (fine with me too!).
5. Given the comment here:
{{{
99 # This function should be detached and included as part of projective
100 # points over number field
101 def abs_log_height(X, gcd_one = True, precision = None):
}}}
I don't think that abs_log_height should be added to the global namespace of sage on startup! Why is that done???? This is caused by comment 1 above. You should just delete that import and in the doctests, put an explicit line to import abs_log_height, e.g.,
{{{
sage: from sage.schemes.elliptic_curves.ell_int_points import abs_log_height
}}}
6. This comment:
{{{
# skipping zero as it currently over K breaks Sage
}}}
Is there a trac ticket for this? What breaks? If there is a ticket, put its number in the comment. If not, open one and put its number in the comment.
7. The Sphinx markup here is wrong:
{{{
- ``gcd_one`` -- (default: True) if false this throws in the
places at the numerators in addition to the places at the denominator.
}}}
To *see* this, I did the following in the notebook:
{{{
import sage.schemes.elliptic_curves.ell_int_points as ellpts
}}}
then
{{{
ellpts.abs_log_height?
}}}
and looked at the docstring output. It should be
{{{
- ``gcd_one`` -- (default: True) if false this throws in the
places at the numerators in addition to the places at the denominator.
}}}
8. I get the following doctest failures when testing your file on my
Linux x86_64 box::
{{{
wstein@ubuntu:~/d/sage$ sage -t schemes/elliptic_curves/ell_int_points.py
sage -t "devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py"
**********************************************************************
File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 10:
sage: E.integral_points()
Expected:
[(13 : -43 : 1), (4 : -16 : 1), (-11 : -19 : 1), (-2 : 29 : 1),
(507525709 : 11433453531221 : 1)]
Got:
[(13 : 29 : 1), (4 : 11 : 1), (-11 : 29 : 1), (-2 : -28 : 1), (507525709 : -11433961056931 : 1)]
**********************************************************************
File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 17:
sage: E.integral_points()
Expected:
[(19 : 74 : 1), (-13 : 266 : 1), (29 : -14 : 1), (22 : -49 : 1), (1639 :
66346 : 1), (71 : 490 : 1), (-27 : 294 : 1), (43 : -154 : 1), (27 : -6 :
1), (53 : 266 : 1), (-41 : -266 : 1), (414 : -8379 : 1), (1667 : -68054
: 1), (23036693 : 110568192854 : 1)]
Got:
[(29 : -14 : 1), (22 : -49 : 1), (-27 : 294 : 1), (53 : 266 : 1), (23036693 : 110568192854 : 1), (-41 : 266 : 1), (27 : 6 : 1), (43 : 154 : 1), (71 : -490 : 1), (1667 : 68054 : 1), (-13 : -266 : 1), (19 : 74 : 1), (1639 : 66346 : 1), (414 : 8379 : 1)]
**********************************************************************
File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 26:
sage: E.integral_points()
Expected:
[(-1 : 0 : 1), (15 : 56 : 1), (3 : 12 : 1), (10 : -44 : 1), (395 : -8052
: 1), (24 : 110 : 1), (0 : -7 : 1), (43 : 264 : 1), (98 : -1023 : 1),
(1681690 : 2179974753 : 1)]
Got:
[(-1 : 0 : 1), (3 : -16 : 1), (10 : 33 : 1), (0 : 6 : 1), (43 : -308 : 1), (1681690 : -2181656444 : 1), (15 : -72 : 1), (395 : 7656 : 1), (24 : 110 : 1), (98 : -1023 : 1)]
**********************************************************************
File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 34:
sage: E.integral_points()
Expected:
[(-7 : 38 : 1), (21 : 66 : 1), (14 : -32 : 1), (-12 : -22 : 1), (175 :
-2398 : 1), (44 : 257 : 1), (-14 : 17 : 1), (33 : -192 : 1), (63 : 458 :
1), (-1 : 22 : 1), (0 : -18 : 1), (98 : -1012 : 1), (13 : -22 : 1),
(1533 : -60792 : 1), (1561 : 60896 : 1), (18888968 : -82103620687 : 1)]
Got:
[(21 : -88 : 1), (-7 : -32 : 1), (14 : 17 : 1), (-12 : 33 : 1), (175 : 2222 : 1), (44 : -302 : 1), (-14 : 17 : 1), (-1 : 22 : 1), (63 : 458 : 1), (33 : -192 : 1), (0 : -18 : 1), (98 : -1012 : 1), (13 : -22 : 1), (1533 : -60792 : 1), (1561 : 60896 : 1), (18888968 : -82103620687 : 1)]
**********************************************************************
1 items had failures:
4 of 14 in __main__.example_0
***Test Failed*** 4 failures.
For whitespace errors, see the file /home/wstein/.sage//tmp/.doctest_ell_int_points.py
[19.8 s]
}}}
9. I'm a bit confused by this comment. What "This function" is it
referring to? Why isn't it just the docstring of a function?
{{{
889
890 # Compute all integral points on elliptic curve over a number field
891 # This function simply computes a suitable bound Q, and return
892 # IntegralPoints(E, L, Q tol = ...).
893 # Input E = elliptic curve over a number field K
894 # L = a sequence of points giving a basis for Mordell-Weil group for E(K)
895 # Output S1 = sequence of all integral points on E(K) modulo [-1]
896 # S2 = sequence of tuples representing the points as a
897 # linear combination of points in L
898 # Option tol = tolerance used for checking integrality of points.
899 # (Default = 0 - only exact arithmetic will be performed)
}}}
10. You need a 100% coverage score, but get only 84%:
{{{
wstein@ubuntu:~/d/sage$ sage -coverage schemes/elliptic_curves/ell_int_points.py
----------------------------------------------------------------------
schemes/elliptic_curves/ell_int_points.py
SCORE schemes/elliptic_curves/ell_int_points.py: 84% (16 of 19)
Missing documentation:
* abs_log_height(X, gcd_one = True, precision = None):
* _integral_points_with_Q(E, L, Q, tol = 0):
* integral_points(self, L=None, tol = 0, both_signs = False):
}}}
11. Did you do any systematic testing, e.g., that this code agrees
with Magma for all curves of conductor up to 1000 or 10000? This
usually uncovers dozens of bugs...
12. All this markup is wrong, for the same reason as above:
{{{
1025 - ``tol`` -- (default: 0, only exact arithmetic will be performed) tolerance used for checking integrality of points, small
1026 number >= 0. Set tolerance - This should be larger than 10**(-current precision) to avoid missing any integral
1027 points. Too large tolerance will slow the computation, too small tolerance may lead to missing some integral points.
1028
1029 - ``both_signs`` -- (default: False) This gives the entire set of points, P and -P for each integral point. If set to True,
1030 this just gives the P and not -P.
}}}
13. Why is this line here? Just delete it.
{{{
#id = self([0,1,0])
}}}
14. I'm *very* concerned about this:
{{{
31 - Alyson Deines, R. Andrew Ohana, Jen Balakrishnan (2011): removed integral_points in
32 preference for the more general number field method
}}}
You could easily have just made Sage massively slower and finding
integral points. OK, so the bounds were evidently wrong so integral
points over QQ wasn't always right. However, that code was pretty
fast I think, since it wasn't too general, etc. Anyways, please at
least *check* what you've done to see what the impact is. My
understanding (from hearing Cremona talk) was that the implementation
you just put in (in this patch) was supposed to be massively slower
than what was in Sage before. Maybe it isn't true. But it is scary
for you to not worry about.