1 | | I checked that the new patch applies fine and passes tests (all tests in sage/schemes/elliptic_curves). I want to do more testing now, to test on a large range of conductors that (over QQ) we find the same integral points as Magma. In any case, I intend to run the new function on all curve of conductor <300000 in order to correct the files at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/data/, so I may as well do all that as part of this review. |

| 1 | I checked that the new patch applies fine and passes most tests (all tests in sage/schemes/elliptic_curves except one, see below). I want to do more testing now, to test on a large range of conductors that (over QQ) we find the same integral points as Magma. In any case, I intend to run the new function on all curve of conductor <300000 in order to correct the files at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/data/, so I may as well do all that as part of this review. |

| 2 | |

| 3 | This seems to be simply a formatting difference: |

| 4 | {{{ |

| 5 | Failed example: |

| 6 | E.integral_points(L = [E((4,-4)),E((0,-20)),E((-4,-28)),E((-89/5,-637*a/25))]) # long time |

| 7 | Expected: |

| 8 | [(-12 , 4 , 1), (-8 , 28 , 1), (-7 , -29 , 1), (-4 , -28 , 1), |

| 9 | (0 , -20 , 1), (1 , 17 , 1), (4 , -4 , 1), (8 , 4 , 1), (9 , -11 , 1), |

| 10 | (12 , -28 , 1), (16 , 52 , 1), (25 , -115 , 1), (32 , 172 , 1), |

| 11 | (44 , 284 , 1), (56 , -412 , 1), (84 , -764 , 1), (148 , 1796 , 1), |

| 12 | (208 , 2996 , 1), (372 , -7172 , 1), (1368 , -50596 , 1), |

| 13 | (1624 , -65444 , 1), (3264 , 186476 , 1)] |

| 14 | Got: |

| 15 | [(-12 : 4 : 1), (-8 : 28 : 1), (-7 : -29 : 1), (-4 : -28 : 1), (0 : -20 : 1), (1 : 17 : 1), (4 : -4 : 1), (8 : 4 : 1), (9 : -11 : 1), (12 : -28 : 1), (16 : 52 : 1), (25 : -115 : 1), (32 : 172 : 1), (44 : 284 : 1), (56 : -412 : 1), (84 : -764 : 1), (148 : 1796 : 1), (208 : 2996 : 1), (372 : -7172 : 1), (1368 : -50596 : 1), (1624 : -65444 : 1), (3264 : 186476 : 1)] |

| 16 | ********************************************************************** |

| 17 | 1 item had failures: |

| 18 | 1 of 14 in sage.schemes.elliptic_curves.ell_int_points.integral_points |

| 19 | [150 tests, 1 failure, 325.30 s] |

| 20 | }}} |