Ticket #10973: report.txt

File report.txt, 8.5 KB (added by William Stein, 11 years ago)

This is a list of 14 issues I had reading through trac_10973.3.patch.

Line 
1Skimming the file:
2
31. Improve this comment you introduce in sage/schemes/elliptic_curves/all.py
4{{{
5# Temp until we integrate this more fully:
6}}}
7
82. This is a run on sentence:
9{{{
10The following examples are from trac ticket #10152, previously Magma was finding more  integral points on these curves than Sage.
11}}}
12
133. This stuff seems weird in the file {{{ell_int_points.py}}}:
14{{{
15        70      #        File: intpts.sage
16        71      #     Created: Thu Jul 01 04:22 PM 2010 C
17        72      # Last Change: Fri Jul 02 12:51 PM 2010
18        73      # Original Magma Code: Thotsaphon "Nook" Thongjunthug 
19        74      # Sage version: Radoslav Kirov, Jackie Anderson
20}}}
21I 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.
22
234. Since you're adding a whole new file that I wrote none of, using this in the copyright file is weird:
24{{{
25#       Copyright (C) 2005,2006,2007 William Stein <wstein@gmail.com>
26}}}
27You could assign the copyright to me (I'm OK with that).  But make the date 2011.  Or keep the copyright (fine with me too!).
28
295. Given the comment here:
30{{{
31         99     # This function should be detached and included as part of projective
32        100     #  points over number field
33        101     def abs_log_height(X, gcd_one = True, precision = None):
34}}}
35I 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.,
36{{{
37sage: from sage.schemes.elliptic_curves.ell_int_points import abs_log_height
38}}}
39
406. This comment:
41{{{
42# skipping zero as it currently over K breaks Sage
43}}}
44Is 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.
45
467. The Sphinx markup here is wrong:
47{{{
48    - ``gcd_one`` -- (default: True) if false this throws in the
49                     places at the numerators in addition to the places at the denominator.
50}}}
51To *see* this, I did the following in the notebook:
52{{{
53import sage.schemes.elliptic_curves.ell_int_points as ellpts   
54}}}
55then
56{{{
57ellpts.abs_log_height?
58}}}
59and looked at the docstring output.  It should be
60{{{
61    - ``gcd_one`` -- (default: True) if false this throws in the
62      places at the numerators in addition to the places at the denominator.
63}}}
64
658.  I get the following doctest failures when testing your file on my
66Linux x86_64 box::
67{{{
68wstein@ubuntu:~/d/sage$ sage -t schemes/elliptic_curves/ell_int_points.py
69sage -t  "devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py"
70**********************************************************************
71File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 10:
72    sage: E.integral_points()
73Expected:
74    [(13 : -43 : 1), (4 : -16 : 1), (-11 : -19 : 1), (-2 : 29 : 1),
75    (507525709 : 11433453531221 : 1)]
76Got:
77    [(13 : 29 : 1), (4 : 11 : 1), (-11 : 29 : 1), (-2 : -28 : 1), (507525709 : -11433961056931 : 1)]
78**********************************************************************
79File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 17:
80    sage: E.integral_points()
81Expected:
82    [(19 : 74 : 1), (-13 : 266 : 1), (29 : -14 : 1), (22 : -49 : 1), (1639 :
83    66346 : 1), (71 : 490 : 1), (-27 : 294 : 1), (43 : -154 : 1), (27 : -6 :
84    1), (53 : 266 : 1), (-41 : -266 : 1), (414 : -8379 : 1), (1667 : -68054
85    : 1), (23036693 : 110568192854 : 1)]
86Got:
87    [(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)]
88**********************************************************************
89File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 26:
90    sage: E.integral_points()
91Expected:
92    [(-1 : 0 : 1), (15 : 56 : 1), (3 : 12 : 1), (10 : -44 : 1), (395 : -8052
93    : 1), (24 : 110 : 1), (0 : -7 : 1), (43 : 264 : 1), (98 : -1023 : 1),
94    (1681690 : 2179974753 : 1)]
95Got:
96    [(-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)]
97**********************************************************************
98File "/home/wstein/sage/sage-4.7.2.alpha2/devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py", line 34:
99    sage: E.integral_points()
100Expected:
101    [(-7 : 38 : 1), (21 : 66 : 1), (14 : -32 : 1), (-12 : -22 : 1), (175 :
102    -2398 : 1), (44 : 257 : 1), (-14 : 17 : 1), (33 : -192 : 1), (63 : 458 :
103    1), (-1 : 22 : 1), (0 : -18 : 1), (98 : -1012 : 1), (13 : -22 : 1),
104    (1533 : -60792 : 1), (1561 : 60896 : 1), (18888968 : -82103620687 : 1)]
105Got:
106    [(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)]
107**********************************************************************
1081 items had failures:
109   4 of  14 in __main__.example_0
110***Test Failed*** 4 failures.
111For whitespace errors, see the file /home/wstein/.sage//tmp/.doctest_ell_int_points.py
112         [19.8 s]
113 
114}}}
115
116
1179. I'm a bit confused by this comment.  What "This function" is it
118referring to?  Why isn't it just the docstring of a function?
119{{{
120889         
121        890     # Compute all integral points on elliptic curve over a number field
122        891     # This function simply computes a suitable bound Q, and return
123        892     # IntegralPoints(E, L, Q    tol = ...). 
124        893     # Input        E = elliptic curve over a number field K
125        894     #              L = a sequence of points giving a basis for Mordell-Weil group for E(K)
126        895     # Output    S1 = sequence of all integral points on E(K) modulo [-1]
127        896     #           S2 = sequence of tuples representing the points as a
128        897     #                linear combination of points in L
129        898     # Option tol = tolerance used for checking integrality of points.
130        899     #                             (Default = 0 - only exact arithmetic will be performed) 
131}}}
132
133
13410. You need a 100% coverage score, but get only 84%:
135{{{
136wstein@ubuntu:~/d/sage$ sage -coverage schemes/elliptic_curves/ell_int_points.py
137----------------------------------------------------------------------
138schemes/elliptic_curves/ell_int_points.py
139SCORE schemes/elliptic_curves/ell_int_points.py: 84% (16 of 19)
140
141Missing documentation:
142         * abs_log_height(X, gcd_one = True, precision = None):
143         * _integral_points_with_Q(E, L, Q, tol = 0):
144         * integral_points(self, L=None, tol = 0, both_signs = False):
145}}}
146
14711. Did you do any systematic testing, e.g., that this code agrees
148with Magma for all curves of conductor up to 1000 or 10000?  This
149usually uncovers dozens of bugs...
150
15112. All this markup is wrong, for the same reason as above:
152{{{
153        1025        - ``tol`` -- (default: 0, only exact arithmetic will be performed) tolerance used for checking integrality of points, small
154        1026                     number >= 0. Set tolerance - This should be larger than 10**(-current precision) to avoid missing any integral 
155        1027                     points. Too large tolerance will slow the computation, too small tolerance may lead to missing some integral points.
156        1028         
157        1029        - ``both_signs`` -- (default: False) This gives the entire set of points, P and -P for each integral point.  If set to True,
158        1030                            this just gives the P and not -P.
159}}}
160
16113. Why is this line here?  Just delete it.
162{{{
163#id = self([0,1,0])
164}}}
165
16614. I'm *very* concerned about this:
167{{{
168        31      - Alyson Deines, R. Andrew Ohana, Jen Balakrishnan (2011): removed integral_points in
169        32        preference for the more general number field method
170}}}
171
172You could easily have just made Sage massively slower and finding
173integral points.  OK, so the bounds were evidently wrong so integral
174points over QQ wasn't always right.  However, that code was pretty
175fast I think, since it wasn't too general, etc.  Anyways, please at
176least *check* what you've done to see what the impact is.  My
177understanding (from hearing Cremona talk) was that the implementation
178you just put in (in this patch) was supposed to be massively slower
179than what was in Sage before.  Maybe it isn't true.  But it is scary
180for you to not worry about.