# Ticket #10973: report.txt

File report.txt, 8.5 KB (added by , 11 years ago) |
---|

Line | |
---|---|

1 | Skimming the file: |

2 | |

3 | 1. Improve this comment you introduce in sage/schemes/elliptic_curves/all.py |

4 | {{{ |

5 | # Temp until we integrate this more fully: |

6 | }}} |

7 | |

8 | 2. This is a run on sentence: |

9 | {{{ |

10 | The following examples are from trac ticket #10152, previously Magma was finding more integral points on these curves than Sage. |

11 | }}} |

12 | |

13 | 3. 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 | }}} |

21 | 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. |

22 | |

23 | 4. 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 | }}} |

27 | 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!). |

28 | |

29 | 5. 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 | }}} |

35 | 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., |

36 | {{{ |

37 | sage: from sage.schemes.elliptic_curves.ell_int_points import abs_log_height |

38 | }}} |

39 | |

40 | 6. This comment: |

41 | {{{ |

42 | # skipping zero as it currently over K breaks Sage |

43 | }}} |

44 | 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. |

45 | |

46 | 7. 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 | }}} |

51 | To *see* this, I did the following in the notebook: |

52 | {{{ |

53 | import sage.schemes.elliptic_curves.ell_int_points as ellpts |

54 | }}} |

55 | then |

56 | {{{ |

57 | ellpts.abs_log_height? |

58 | }}} |

59 | and 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 | |

65 | 8. I get the following doctest failures when testing your file on my |

66 | Linux x86_64 box:: |

67 | {{{ |

68 | wstein@ubuntu:~/d/sage$ sage -t schemes/elliptic_curves/ell_int_points.py |

69 | sage -t "devel/sage-main/sage/schemes/elliptic_curves/ell_int_points.py" |

70 | ********************************************************************** |

71 | File "/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() |

73 | Expected: |

74 | [(13 : -43 : 1), (4 : -16 : 1), (-11 : -19 : 1), (-2 : 29 : 1), |

75 | (507525709 : 11433453531221 : 1)] |

76 | Got: |

77 | [(13 : 29 : 1), (4 : 11 : 1), (-11 : 29 : 1), (-2 : -28 : 1), (507525709 : -11433961056931 : 1)] |

78 | ********************************************************************** |

79 | File "/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() |

81 | Expected: |

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)] |

86 | Got: |

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 | ********************************************************************** |

89 | File "/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() |

91 | Expected: |

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)] |

95 | Got: |

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 | ********************************************************************** |

98 | File "/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() |

100 | Expected: |

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)] |

105 | Got: |

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 | ********************************************************************** |

108 | 1 items had failures: |

109 | 4 of 14 in __main__.example_0 |

110 | ***Test Failed*** 4 failures. |

111 | For whitespace errors, see the file /home/wstein/.sage//tmp/.doctest_ell_int_points.py |

112 | [19.8 s] |

113 | |

114 | }}} |

115 | |

116 | |

117 | 9. I'm a bit confused by this comment. What "This function" is it |

118 | referring to? Why isn't it just the docstring of a function? |

119 | {{{ |

120 | 889 |

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 | |

134 | 10. You need a 100% coverage score, but get only 84%: |

135 | {{{ |

136 | wstein@ubuntu:~/d/sage$ sage -coverage schemes/elliptic_curves/ell_int_points.py |

137 | ---------------------------------------------------------------------- |

138 | schemes/elliptic_curves/ell_int_points.py |

139 | SCORE schemes/elliptic_curves/ell_int_points.py: 84% (16 of 19) |

140 | |

141 | Missing 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 | |

147 | 11. Did you do any systematic testing, e.g., that this code agrees |

148 | with Magma for all curves of conductor up to 1000 or 10000? This |

149 | usually uncovers dozens of bugs... |

150 | |

151 | 12. 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 | |

161 | 13. Why is this line here? Just delete it. |

162 | {{{ |

163 | #id = self([0,1,0]) |

164 | }}} |

165 | |

166 | 14. 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 | |

172 | You could easily have just made Sage massively slower and finding |

173 | integral points. OK, so the bounds were evidently wrong so integral |

174 | points over QQ wasn't always right. However, that code was pretty |

175 | fast I think, since it wasn't too general, etc. Anyways, please at |

176 | least *check* what you've done to see what the impact is. My |

177 | understanding (from hearing Cremona talk) was that the implementation |

178 | you just put in (in this patch) was supposed to be massively slower |

179 | than what was in Sage before. Maybe it isn't true. But it is scary |

180 | for you to not worry about. |