Opened 8 years ago
Last modified 3 days ago
#10973 needs_work enhancement
Integral points on elliptic curves over number fields
Reported by:  justin  Owned by:  cremona 

Priority:  major  Milestone:  sage8.9 
Component:  elliptic curves  Keywords:  sd32 
Cc:  cremona, gagansekhon, jen  Merged in:  
Authors:  Justin Walker, Aly Deines, Jennifer Balakrishnan  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  public/10973 (Commits)  Commit:  0774d5990a14f8b923ec6f876891f5f8ee871487 
Dependencies:  Stopgaps: 
Description (last modified by )
Incorporate work done by Rado Kirov and Jackie Anderson at Sage Days 22, based on Magma implementation by Cremona's student Nook.
See also #12095 (now tagged as duplicate).
Converted to git by John Cremona, 20131218
Attachments (14)
Change History (89)
Changed 8 years ago by
comment:1 Changed 8 years ago by
 Type changed from PLEASE CHANGE to enhancement
Changed 8 years ago by
comment:2 Changed 8 years ago by
New version of .py file, fixing a problem with bogus interaction with Maxima.
Changed 8 years ago by
comment:3 Changed 8 years ago by
The patch trac_10973 should apply w/o problems against sage4.7.alpha1.
The previous attachments are replaced by this one.
comment:4 Changed 8 years ago by
This patch is related to, and appears to fix, the problem described by #10152.
Changed 8 years ago by
Updated patch with code to reject requests w/o generators; replaces previous patch of same name.
comment:5 Changed 8 years ago by
minor error in all.py, +from ell_int_points.py import *
should be +from ell_int_points import *
The new patch that I will be uploading in a minute fixes that.
Changed 8 years ago by
comment:6 Changed 8 years ago by
 Cc gagansekhon added
comment:7 Changed 8 years ago by
comment:8 Changed 8 years ago by
 Status changed from new to needs_review
This should do it. This is a small fix to the previous patch, it now passes the tests.
comment:9 Changed 8 years ago by
comment:10 Changed 8 years ago by
comment:11 Changed 8 years ago by
 Status changed from needs_review to needs_work
Please see the rather long file report.txt that I posted with a bunch of issues I noticed when reading through trac_10973.3.patch.
comment:12 followup: ↓ 13 Changed 8 years ago by
 Status changed from needs_work to needs_review
Trac10973.4.patch addresses everything in report.txt.
This returns the same result as the original integral_points over Q
with all curves of conductor up to 1000 (and also agrees with Magma). This also finds the missing points referred to in ticket #10152.
We compared times on all curves over Q
of conductor less then 1000; the original implementation was usually 2 to 8
times faster, though in certain instances it did not find all integral points (this was ticket #10152). So this is a bit of a slowdown.
If E
is a curve over Q
, then E.integral_points() calls the method in ell_rational_points.py (the previous, faster, not always correct, method).
comment:13 in reply to: ↑ 12 Changed 8 years ago by
Replying to aly.deines:
If
E
is a curve overQ
, then E.integral_points() calls the method in ell_rational_points.py (the previous, faster, not always correct, method).
Why? Surely it would definitely be better to return a correct answer by default. I can imagine leaving the old code in with an notbydefault option to call it and docs that mention it is faster.
comment:14 followup: ↓ 17 Changed 8 years ago by
Here's a list of curves where your code and the latest (?) Magma V2.174 give different answers. I checked these by hand, and Magma is clearly totally wrong (missing definitely integral points) in each case.
1264i1, 1328b1, 1656f1, 1848i1
comment:15 Changed 8 years ago by
 Cc jen added
comment:16 Changed 8 years ago by
 Keywords sd32 added
comment:17 in reply to: ↑ 14 Changed 8 years ago by
Replying to was:
Here's a list of curves where your code and the latest (?) Magma V2.174 give different answers. I checked these by hand, and Magma is clearly totally wrong (missing definitely integral points) in each case.
1264i1, 1328b1, 1656f1, 1848i1
Update. Using Sage4.7.2 and Magma V2.179, these all agree:
sage: E = EllipticCurve("1264i1") sage: E.integral_points() [(2 : 0 : 1), (4 : 10 : 1), (18 : 80 : 1), (246404 : 122312970 : 1)] sage: magma(E).IntegralPoints() [ (2 : 0 : 1), (4 : 10 : 1), (18 : 80 : 1), (246404 : 122312970 : 1) ] sage: E = EllipticCurve("1328b1") sage: E.integral_points() [(2 : 2 : 1), (4386 : 290438 : 1)] sage: magma(E).IntegralPoints() [ (2 : 2 : 1), (4386 : 290438 : 1) ] sage: E = EllipticCurve("1656f1") sage: E.integral_points() [(3 : 0 : 1), (6 : 18 : 1), (27 : 144 : 1), (336678 : 195353910 : 1)] sage: magma(E).IntegralPoints() [ (3 : 0 : 1), (6 : 18 : 1), (27 : 144 : 1), (336678 : 195353910 : 1) ] sage: E = EllipticCurve("1848i1") sage: E.integral_points() [(2 : 3 : 1), (3206 : 181557 : 1)] sage: magma(E).IntegralPoints() [ (2 : 3 : 1), (3206 : 181557 : 1) ]
That is using vanilla Sage (i.e. without any patches from this ticket). I am about to do a more systematic test.
comment:18 followup: ↓ 20 Changed 8 years ago by
I am testing this now. It looks as if all the points raised in William's last report have been dealt with.
It looks odd to me to have the main integral_points function which is a method of the EllipticCurve_number_field class defined in a separate file, so that in ell_number_field.py you only see the line integral_points = integral_points. I did not know that would work! But, given that it does, it is cleaner. I will check to see that the documentation is not thereby hidden.
comment:19 Changed 8 years ago by
 Description modified (diff)
 Status changed from needs_review to positive_review
Positive review. In my reviewer's patch I fix the following:
 Now applies to 4.7.2 with no fuzz.
 Added QQ to import list on line 332 of ell_number_field.py, otherwise testing ell_int_pts failed, but testing ell_number_field did not  since the test for that function silverman_height_bounds was incomplete! I fixed that too.
 Fixed a warning in docbuild by adding a blank line at line 69 of the same file.
It is now true that for an elliptic curve E over Q we have both the new E.silverman_height_bounds(), which gives lower and upper bounds, and the old E.silverman_height_bound(), which only gives one of them. The latter can be deleted but that will require some tweaking of any code which uses it. I have left that for another ticket.
comment:20 in reply to: ↑ 18 ; followup: ↓ 21 Changed 8 years ago by
Replying to cremona:
I am testing this now. It looks as if all the points raised in William's last report have been dealt with.
The first curve where Sage4.7.2 fails to find the same number of integral points as Magma V2.179 is 2082a1 where Sage4.7.2 finds xcoordinates 11,2,4,13 but not 507525709 (!). But the new version does find that last one.
I will carry on testing, but there is no need to delay merging this (expect perhaps if I find a curve for which the new code fails to find all known points).
comment:21 in reply to: ↑ 20 Changed 8 years ago by
Replying to cremona:
Replying to cremona:
I am testing this now. It looks as if all the points raised in William's last report have been dealt with.
The first curve where Sage4.7.2 fails to find the same number of integral points as Magma V2.179 is 2082a1 where Sage4.7.2 finds xcoordinates 11,2,4,13 but not 507525709 (!). But the new version does find that last one.
I will carry on testing, but there is no need to delay merging this (expect perhaps if I find a curve for which the new code fails to find all known points).
For all curves of conductor up to 10000, there are precisely 3 where Sage4.7.2 misses integral points compared with Magma V2.179, namely 2082a1, 6104b1, 8470g1, 8470g2; in each case just one point is missed. And the new code agrees with Magma in all these cases. Regarding timings, up to 10000 Sage4.7.2 took 157m while Magma took 943m; that may be an unfair comparison though since Sage took generators from the DB while Magma (I think) computed them from scratch. I will redo this with the current patch applied.
comment:22 Changed 8 years ago by
 Description modified (diff)
 Milestone set to sage4.8
 Reviewers set to John Cremona
comment:23 Changed 8 years ago by
 Status changed from positive_review to needs_work
I unfortunately get several doctest failures (with sage4.8.alpha2 + various patches). Did you test this patch with the new PARI (merged in sage4.8.alpha1)? I am also afraid that there are speed regressions, I am seeing a timeout:
sage t force_lib devel/sage/sage/schemes/elliptic_curves/ell_int_points.py ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage4.8.alpha3/devel/sagemain/sage/schemes/elliptic_curves/ell_int_points.py", line 12: sage: E.integral_points() Expected: [(11 : 19 : 1), (2 : 29 : 1), (4 : 16 : 1), (13 : 43 : 1), (507525709 : 11433453531221 : 1)] Got: [(11 : 29 : 1), (2 : 28 : 1), (4 : 11 : 1), (13 : 29 : 1), (507525709 : 11433961056931 : 1)] ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage4.8.alpha3/devel/sagemain/sage/schemes/elliptic_curves/ell_int_points.py", line 22: sage: E.integral_points() Expected: [(41 : 266 : 1), (27 : 294 : 1), (13 : 266 : 1), (19 : 74 : 1), (22 : 49 : 1), (27 : 6 : 1), (29 : 14 : 1), (43 : 154 : 1), (53 : 266 : 1), (71 : 490 : 1), (414 : 8379 : 1), (1639 : 66346 : 1), (1667 : 68054 : 1), (23036693 : 110568192854 : 1)] Got: [(41 : 266 : 1), (27 : 294 : 1), (13 : 266 : 1), (19 : 74 : 1), (22 : 49 : 1), (27 : 6 : 1), (29 : 14 : 1), (43 : 154 : 1), (53 : 266 : 1), (71 : 490 : 1), (414 : 8379 : 1), (1639 : 66346 : 1), (1667 : 68054 : 1), (23036693 : 110568192854 : 1)] ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage4.8.alpha3/devel/sagemain/sage/schemes/elliptic_curves/ell_int_points.py", line 31: sage: E.integral_points(algorithm = "old") Expected: [(1 : 0 : 1), (0 : 6 : 1), (3 : 12 : 1), (10 : 33 : 1), (15 : 56 : 1), (24 : 110 : 1), (43 : 264 : 1), (98 : 924 : 1), (395 : 7656 : 1)] Got: [(1 : 0 : 1), (0 : 6 : 1), (3 : 12 : 1), (10 : 33 : 1), (15 : 56 : 1), (24 : 110 : 1), (43 : 264 : 1), (98 : 924 : 1), (395 : 7656 : 1), (1681690 : 2179974753 : 1)] ********************************************************************* File "/mnt/usb1/scratch/jdemeyer/merger/sage4.8.alpha3/devel/sagemain/sage/schemes/elliptic_curves/ell_int_points.py", line 34: sage: E.integral_points() Expected: [(1 : 0 : 1), (0 : 7 : 1), (3 : 12 : 1), (10 : 44 : 1), (15 : 56 : 1), (24 : 110 : 1), (43 : 264 : 1), (98 : 1023 : 1), (395 : 8052 : 1), (1681690 : 2179974753 : 1)] Got: [(1 : 0 : 1), (0 : 6 : 1), (3 : 16 : 1), (10 : 33 : 1), (15 : 72 : 1), (24 : 110 : 1), (43 : 308 : 1), (98 : 1023 : 1), (395 : 76 56 : 1), (1681690 : 2181656444 : 1)] ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage4.8.alpha3/devel/sagemain/sage/schemes/elliptic_curves/ell_int_points.py", line 46: sage: E.integral_points() Expected: [(14 : 17 : 1), (12 : 22 : 1), (7 : 38 : 1), (1 : 22 : 1), (0 : 18 : 1), (13 : 22 : 1), (14 : 32 : 1), (21 : 66 : 1), (33 : 192 : 1), (44 : 257 : 1), (63 : 458 : 1), (98 : 1012 : 1), (175 : 2398 : 1), (1533 : 60792 : 1), (1561 : 60896 : 1), (18888968 : 82103620687 : 1)] Got: [(14 : 17 : 1), (12 : 33 : 1), (7 : 32 : 1), (1 : 22 : 1), (0 : 18 : 1), (13 : 22 : 1), (14 : 17 : 1), (21 : 88 : 1), (33 : 1 92 : 1), (44 : 302 : 1), (63 : 458 : 1), (98 : 1012 : 1), (175 : 2222 : 1), (1533 : 60792 : 1), (1561 : 60896 : 1), (18888968 : 821036 20687 : 1)] ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage4.8.alpha3/devel/sagemain/sage/schemes/elliptic_curves/ell_int_points.py", line 131: sage: ellpts.abs_log_height(list(P2)) Expected: 1.09861228866811 Got: 3.21887582486820 **********************************************************************
The following tests failed: sage t force_lib devel/sage/doc/en/bordeaux_2008/elliptic_curves.rst # Time out sage t force_lib devel/sage/sage/schemes/elliptic_curves/ell_int_points.py # 6 doctests failed
comment:24 Changed 8 years ago by
Minor comment: could you format the copyright notice of the added file as shown in http://sagemath.org/doc/developer/conventions.html#headingsofsagelibrarycodefiles. I am also very surprised to see William Stein in the copyright statement, he is not even an author of this ticket.
comment:25 Changed 8 years ago by
Line 617: replace by initQ = 10^118
:)
comment:26 Changed 8 years ago by
If you wordwrap doctest results (which is a good idea), try to wordwrap them more logically, i.e. write
[(41 : 266 : 1), (27 : 294 : 1), (13 : 266 : 1), (19 : 74 : 1), (22 : 49 : 1), (27 : 6 : 1), (29 : 14 : 1), (43 : 154 : 1), (53 : 266 : 1), (71 : 490 : 1), (414 : 8379 : 1), (1639 : 66346 : 1), (1667 : 68054 : 1), (23036693 : 110568192854 : 1)]
instead of
[(41 : 266 : 1), (27 : 294 : 1), (13 : 266 : 1), (19 : 74 : 1), (22 : 49 : 1), (27 : 6 : 1), (29 : 14 : 1), (43 : 154 : 1), (53 : 266 : 1), (71 : 490 : 1), (414 : 8379 : 1), (1639 : 66346 : 1), (1667 : 68054 : 1), (23036693 : 110568192854 : 1)]
comment:27 Changed 8 years ago by
In the documentation, you refer to ticket 10152. This should be 10973 instead.
comment:28 Changed 8 years ago by
Indentation at the top should probably be
r""" Integral Points of Elliptic Curves over Number Fields The following examples are from trac ticket #10152. Previously Magma was finding ore integral points on these curves than Sage. We resolved this issue. EXAMPLES:: sage: E = EllipticCurve('2082a1') sage: E.integral_points(algorithm = "old") [(11 : 29 : 1), (2 : 29 : 1), (4 : 11 : 1), (13 : 29 : 1)] sage: E.integral_points() [(11 : 19 : 1), (2 : 29 : 1), (4 : 16 : 1), (13 : 43 : 1), (507525709 : 11433453531221 : 1)] :: sage: ...
comment:29 Changed 8 years ago by
Since setting this to positive review I have done more testing and was about to revert it to "needs work" myself anyway: in a test of all curves to conductor 10000 I found that the new code appears to hang on 9615d1.
comment:30 Changed 8 years ago by
 Description modified (diff)
 Milestone sage4.8 deleted
 Reviewers John Cremona deleted
See comments at #12095
comment:31 followup: ↓ 33 Changed 8 years ago by
Did you intentionally remove the reviewer and milestone fields? If you did, I don't understand why.
comment:32 Changed 7 years ago by
For the patchbot: apply Trac10973.7.patch
comment:33 in reply to: ↑ 31 Changed 7 years ago by
Replying to jdemeyer:
Did you intentionally remove the reviewer and milestone fields? If you did, I don't understand why.
I did not (at least, not intentionally).
comment:34 Changed 7 years ago by
Noting that one effect of this code is to improve integralpointfinding over QQ (though it is still not always correct, see comment 29 above, we should at least add a stopgap trigger for integral points over QQ, to alert the user to known issues. Example: without this patch,
sage: E = EllipticCurve("2082a1") sage: P = E.gens()[0] sage: E.integral_points() [(11 : 29 : 1), (2 : 29 : 1), (4 : 11 : 1), (13 : 29 : 1)] sage: 13*P (507525709 : 11433453531221 : 1)
comment:35 Changed 6 years ago by
 Description modified (diff)
patchbot, apply only trac_10973_v8.patch
comment:36 Changed 6 years ago by
9 doctests are failing in sage/schemes/elliptic_curves/ell_int_points.py
the failing test in _c3 seems to be a matter of precision only
The other tests must be checked mathematically.
comment:37 Changed 6 years ago by
apply only trac_10973_v8.patch
comment:38 Changed 6 years ago by
This ticket needs care from someone who can check the failing doctests.
comment:39 Changed 6 years ago by
apply only trac_10973_v8.patch
comment:40 Changed 6 years ago by
I have corrected the (short) failing doctests, and inserted the file in the doc.
There may remain one (very) long doctest failing.
comment:41 Changed 6 years ago by
 Milestone set to sage5.13
 Status changed from needs_work to needs_review
I have corrected the last failing doctest
apply only trac_10973_v8.patch
Needs review !
comment:42 Changed 6 years ago by
 Description modified (diff)
comment:43 Changed 6 years ago by
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.
This seems to be simply a formatting difference:
Failed example: E.integral_points(L = [E((4,4)),E((0,20)),E((4,28)),E((89/5,637*a/25))]) # long time Expected: [(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)] Got: [(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)] ********************************************************************** 1 item had failures: 1 of 14 in sage.schemes.elliptic_curves.ell_int_points.integral_points [150 tests, 1 failure, 325.30 s]
comment:44 Changed 6 years ago by
More seriously,
sage: E = EllipticCurve('9615d1) sage: E.integral_points()
seems to hang, thought I don't know why, while
sage: E = EllipticCurve('9615d1) sage: E.integral_points(algorithm="old")
is fine. I think that this will need investigating before this can be merged. Hence "needs work".
I systematically checked all curves with conductor < 10000 with Magma V2.198. Note that thaere have been a lot of improvements in Magma's algorithm, due to Steve DOnelly but not published, since this ticket was opened. Not just bugfixes; but only over Q.
With the old algorithm in Sage for N<10000 there are only 3 cases where Sage missed integral points. Of these, 3 (6104b1, 8470g1 and 8470g2), are very easily fixed, simply by increasing the precision of the function integral_points_with_bounded_mw_coeffs
from 100 to 200, thogh I have not done the necessary analysis to compute the actual precision required. The other one is 2082a1, where the MW group is cyclic of rank 1 and the integral multiples of the generator P are n*P for n=1,2,3,4,13, but the bound computed by the algorithm is 12. It should be possible to find that bug using a detailed analysis of the implementation, but I have not done that yet (though I did find one small bug, in the formula for mu there should be b2/12 and not b2).
comment:45 Changed 6 years ago by
I have done some debugging, not yet complete but I'll be away for a while and wanted to record what I found so far.
2082a1: the old code is not nearly careful enough about computing elliptic logs and periods to high enough precision, and I have worked a bit on improving that, but in this example it reduces the coefficient bound first from 10^44
to 34 (that looks OK to me) and then to 12, which is not OK since the actual bound is 13. And I can see what is done wrong: in Cohen's notation you need a lower bound on d(L,0)^2
which is greater than T^2+Q
, and in this second step it is not; the code makes an adjustment, which is not (as far as I can see) justified. Instead one should increase the scaling parameter C. I found that multiplying C by 100 is OK, but yields a worse bound on the coefficients, namely 56. If one did check all 34 multiples of the generator (the rank is 1) then that would be OK. But the new code here manages to get successive bounds 10^40
, 53, 18 and uses 18 which works better than 34 would have done! The two implementations need further careful comparison.
9615d1, which has rank 2. The new code finds a bound of 162, and the apparent hanging happens when testing all multiples with coefficients up to that bound. This is what is taking a very long time: there are 52812 points to check, and many will have huge height. This is a good illustration of the benefit of using floating point at this stage, since on the same curve if you run integral_points_with_bounded_mw_coeffs(E,E.gens(),162)
it finishes quickly (and finds all the points, namely the 15 which Magma also finds). On the other hand the old code obtains a coefficient bound of only 18, which is good enough, so it is necessary to find out why the new code does so much worse (bound 162).
I don't feel prepared to remove the "needs work" tag just yet.
comment:46 Changed 6 years ago by
I have made a new patch, correcting the one failing doctest
comment:47 Changed 6 years ago by
for the patchbots: apply trac_10973_v8.patch
comment:48 Changed 6 years ago by
I have made a new patch, that should work with 5.13.beta1
for the patchbots: apply trac_10973_v8.patch
comment:49 Changed 6 years ago by
 Branch set to u/cremona/trac10973intpts
 Commit set to 7f5036284d4b31fb81bd69a9bf0a1927c70f3206
 Description modified (diff)
 Status changed from needs_review to needs_work
Last 10 new commits:
7f50362  Trac #10973: integer points on elliptic curves over number fields 
057b3cd  Merge branch 'master' of git://github.com/sagemath/sage 
68d0a5d  Merge branch 'master' of git://github.com/sagemath/sage 
88ffd55  Merge branch 'master' of git://github.com/sagemath/sage 
0523e0d  Merge branch 'master' of git://github.com/sagemath/sage 
858bb95  Merge branch 'master' of git://github.com/sagemath/sage 
b06bd7c  Merge branch 'master' of git://github.com/sagemath/sage 
17e17dd  Merge branch 'master' of ssh://trac.sagemath.org:2222/sage into build_system 
139dcf5  Merge branch 'build_system' of git://github.com/sagemath/sage into build_system 
38d22b7  Merge branch 'master' of ssh://trac.sagemath.org:2222/sage into build_system 
comment:50 followup: ↓ 51 Changed 5 years ago by
Why is this "needs works" ?
comment:51 in reply to: ↑ 50 Changed 5 years ago by
Replying to chapoton:
Why is this "needs works" ?
Because I am actively working on it! Seriously, comparing in detail Smart's paper and book and Nook's account in his thesis, I have found two things: (1) Smart does not deal properly with torsion generators, Nook does and that is in this code but I think I might be able to do better; (2) more seriously, Smart derived an upper bound for x(P)^2\psi(P)
(relative to each embedding) which only valid for points with x(P)
bounded below by some quantity (in the appropriate embedding), and this means that there are points, possible integral, which fail this condition and need to be considered separately. Over Q that is easily done by searching on a finite xinterval, but over a general number field it is not clear to me that the "missing" points are in a finite region. However, I am working on a bound (which certainly exists) for x(P)^2\psi(P)
valid for all complex points with any embedding, and when that is done I will be changing some of the constants in the code.
I am not expecting the output to be different in any of the examples (but who knows), but I cannot let this in when I have identified but not yet fixed this mathematical bug!
comment:52 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:53 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:54 Changed 5 years ago by
 Commit changed from 7f5036284d4b31fb81bd69a9bf0a1927c70f3206 to 10243adf8021440a184eb36695a00af49a41cb27
comment:55 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:56 Changed 4 years ago by
 Branch changed from u/cremona/trac10973intpts to public/ticket/10973
 Commit changed from 10243adf8021440a184eb36695a00af49a41cb27 to 0c14fa06b0f0b15922f2bb2cab4761e85e962536
 Milestone changed from sage6.4 to sage6.9
comment:57 Changed 4 years ago by
 Commit changed from 0c14fa06b0f0b15922f2bb2cab4761e85e962536 to 3985a09232406547e38a02607a80b217a559c215
comment:58 Changed 4 years ago by
 Milestone changed from sage6.9 to sage6.10
comment:59 Changed 3 years ago by
 Branch changed from public/ticket/10973 to u/cremona/10973
 Commit changed from 3985a09232406547e38a02607a80b217a559c215 to f9208472a60f135532d19deff365f8a45dc8572c
comment:60 Changed 3 years ago by
 Branch changed from u/cremona/10973 to public/10973
 Commit changed from f9208472a60f135532d19deff365f8a45dc8572c to 196ddc194ab3ce9dc19f53dccc4676b2d6fd5753
 Milestone changed from sage6.10 to sage7.4
comment:61 Changed 3 years ago by
 Commit changed from 196ddc194ab3ce9dc19f53dccc4676b2d6fd5753 to 6a1a0b8ec8710a79281e1506fbed7988ef393fc0
comment:62 Changed 3 years ago by
 Commit changed from 6a1a0b8ec8710a79281e1506fbed7988ef393fc0 to 0715968866c6db7f68cb274a6f4bfe4a3f80599e
comment:63 Changed 2 years ago by
 Commit changed from 0715968866c6db7f68cb274a6f4bfe4a3f80599e to 871adda033a43b5bbd34ce1111de6fcdb379b34c
Branch pushed to git repo; I updated commit sha1. New commits:
871adda  Merge branch 'public/10973' in 7.6.rc0

comment:64 Changed 2 years ago by
Thanks for keeping this updated. I just have not been able to finish work on it (there are some mathematical issues).
comment:65 Changed 10 months ago by
 Commit changed from 871adda033a43b5bbd34ce1111de6fcdb379b34c to 748216dac716b87b62627ac4b6a229e4f36e8e2c
Branch pushed to git repo; I updated commit sha1. New commits:
748216d  Merge branch 'public/10973' in 8.4.b1

comment:66 Changed 5 months ago by
 Commit changed from 748216dac716b87b62627ac4b6a229e4f36e8e2c to 9cb7356fc96b55c377a3a4a60d72846f91bd266a
Branch pushed to git repo; I updated commit sha1. New commits:
9cb7356  Merge branch 'develop' into 10973

comment:67 Changed 5 months ago by
I have merged the current working branch in with 8.6.beta1 and resolved the conflicts.
I also made the following test: there are 207 curves in the database (conductors < 400000, curves defined over QQ) where the old E.integral_points() missed integral points, as I found out by running every curve through Magma's IntegralPoints? function. For every one of these the current version finds all the points (the same as Magma does).
However I still think that this code is too slow over QQ and intend to make a new branch (and a new ticket #27015) to fix the problems over QQ. When that is done I will get back to this branch.
comment:68 Changed 5 months ago by
Correction to the above: it was true that there were 207 curves whose lists of integral points at http://johncremona.github.io/ecdata/ (since updated) were incomplete, and hence also the integral points stored in the LMFDB (not yet updated). However, in all but 3 of these cases (2082a1, 23808e1, 285928a1) the current Sage (8.5) finds all the points. So previous bug fixes have helped.
In the negative direction there are some more discrepancies, since we now (using vanilla 8.5) miss x=20829482 on 1701d1 and x=176 and 179 on 1848f1. (There will surely be more).
The complete list of missed points with Sage 8.5 is: 12 points on 11 curves:
1701d1, 1848f1, 2082a1, 3510n1, 6514b1, 177211a1, 23808e1, 268740c1, 285928a1, 66432f1, 92554e1,
missing 1 point in each case except 2 points on 1848f1.
#27015 will deal with this.
comment:69 Changed 4 months ago by
 Commit changed from 9cb7356fc96b55c377a3a4a60d72846f91bd266a to 9fe35b9a59cfac0d639be8fb981040231f3494c8
comment:70 Changed 4 months ago by
So what's the status here ? Should we just fix the few failing doctests ?
comment:71 Changed 4 months ago by
 Milestone changed from sage7.4 to sage8.7
comment:72 Changed 4 months ago by
 Commit changed from 9fe35b9a59cfac0d639be8fb981040231f3494c8 to 0774d5990a14f8b923ec6f876891f5f8ee871487
Branch pushed to git repo; I updated commit sha1. New commits:
0774d59  fixing blocks

comment:73 Changed 4 months ago by
I have a current branch called intptsQ which I made good progress on in December and will get back to.
comment:74 Changed 3 months ago by
 Milestone changed from sage8.7 to sage8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:75 Changed 3 days ago by
 Milestone changed from sage8.8 to sage8.9
Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).
This code goes into "sage/schemes/elliptiic_curves". It works, but needs work.