-
-
Notifications
You must be signed in to change notification settings - Fork 453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement 2-isogeny descent over QQ natively in Sage using ratpoints #6583
Comments
comment:2
Although the patch above isn't finished, I do consider it polished for what it does. The next steps are:
Although not in a completely finished state, comments are still welcome! |
comment:4
For my purposes (working within Cremona's big table, N<130000), the dimensions of the subspaces of Q*/(Q*)^2 I am considering are 0: 242 cases Based on this, I do not consider it a priority to do #2. The others are also not necessary for my purposes, so I'm doing some serious triage and getting this into a mergable state now. These other enhancements can come later. |
comment:5
Attachment: trac_6583.patch.gz |
comment:6
Attachment: trac_6583_large_primes.patch.gz |
comment:7
Applying the patch and doing "sage -t" yields some failures, probably because you assume the large cremona database is installed, but it isn't:
|
comment:8
Attachment: trac_6583-fix.patch.gz |
comment:9
Ok. I tested the patches and they all go through. Also the -coverage gives 6 out of 6 despite the fact that many functions in descent_two_isogeny.pyx lack a good documentation and especially examples are missing often. I guess this is because cdefs are not tested. (I admit I never reviewed a cython file.) Currently the main function gives back four integers, like mwrank does. They are useful to compute an upper bound on the rank of the Mordell-Weil group and should (in the future) be called from Personnally I would like more, namely I would like to have a phi-Selmer group. Ideally we should give back a abelian group associated to the elliptic curve, or even better to the isogeny. But isogenies are not there yet, though 2-isogenies should be. The Selmer groups would contain more information than just these four integers and I believe this extra-information is already computed in this algorithm. Right now the functions are not callable without an extra import and so they do not pollute the name-space and we are still flexible in how to do this later; like It is work in progress (and it is very good work in the right direction), but I am incapable of saying whether this sort of work should already be included in sage or not. So before giving a positive review to this patch, I would like to ask the author's and other's opinion on this. Maybe I could also ask for more examples, especially in the header of the file, despite the fact that it is not included in the documentation right now. Chris. |
comment:10
A couple of follow-up points to Chris, from an independent observer: first, can we have some motivation, such as examples where this works better than calling mwrank? There will certainly be curves for which the performance is worse (where mwrank would use a second descent). I suspect that any enhanced perofrmance comes from the use of the more recent version of ratpoints than mwrank uses. Secondly, the isogeny patch I am finishing up will allow 2-isogenies (and more) to be easily constructed (which they almost can already). A very interesting project would be to take any (cyclic) isogeny between elliptic curves, defined over QQ, and return an appropriate Selmer group. |
comment:11
Here's an example where mwrank does not do a second descent, where the runtime is approximately 90 times faster:
I ran the following to compare times in general:
mwrank is always slower by a factor of at least 5, almost always slower by a factor of at least 20, and frequently slower by factors of up to 150. |
comment:12
Maybe this is a more accurate comparison:
Thus the average speedup is 28x. |
comment:13
For the curious, standard deviation:
|
comment:14
Robert -- I am quite prepared to believe that your code is fast, but all these tests on the curves of conductor up to 200 are not exactly typical! I seem to remember that there is one curve in that range which mwrank used to take longer on than all the others put together, for example. |
comment:15
I was just trying to address your question whether there were examples where this was better than mwrank. Your second comment, "any enhanced performance comes from the use of the more recent version of ratpoints" is very false: if I remove the use of ratpoints altogether, the speedup factors increase!
Thus I argue that this code is definitely worth including, especially since this exact code was the main motivation for including ratpoints in Sage in the first place. I don't know what "typical" means regarding the conductor bound, especially since I'm primarily interested in curves with small conductor. I spent a long time optimizing this code, and I'd rather not see that work getting lost to the four winds. (John-- Maybe I'm just missing your point?) |
comment:16
Sorry, I did not mean to sound so critical. Of course this should be included. I have not had time to look at it in detail (though I feel that people expect me to, and give an expert opinion). I think it would be good to have native 2-descent code in Sage (another extremely useful project would be to rewrite Simon's gp program over number fields in Sage), not least because the current interface is not very good (hard to use non-standard parameter settings, for example) and uses the ancient ratpoints, all because I have not had the time to improve those things. Experience tells me that if you put in code which was designed for curves with small conductor then you will very soon find that people try to run the same code on huge examples. This is precisely what happened to me -- the first version of what later became mwrank was written solely for the purpose of computing the ranks of the curves in my book, i.e. N<1000. So the new code must be tested on large examples too. |
comment:17
I've tried the same benchmarks on curves in the conductor range 129800 to 130000. There the average speedup is 117x. Once I have the Stein-Watkins database downloaded, I can try running some examples from there. I'd like to attempt to summarize the referee comments so far:
|
comment:19
REPORT:
The rebase is trivial, and I've posted it.
In summary: I've applied the patches, skimmed them, and run the test suite. Positive review. |
Attachment: trac_6583-rebase.patch.gz |
comment:20
I am very happy to endorse this decision. When I looked at this some time ago I think I found some extra functionality with the ratpoints package which should be, but is not yet, exposed, and started to work on that, but got diverted into other things. Never mind: we can always build on this and extra things if and when we want to. Good, job, Robert! |
Reviewer: William Stein |
Merged: sage-4.3.1.alpha0 |
comment:22
Could you please take a look at #7867 which is a ticket indicating a problem building Sage on 't2' which could possibly be related to this. |
comment:23
For the record, this is definitely the code which has broken the Solaris build. Minh has proven this: http://groups.google.com/group/sage-devel/msg/bb1da5365b7f1c90 Dave |
CC: @JohnCremona @williamstein
Component: elliptic curves
Author: Robert Miller
Reviewer: William Stein
Merged: sage-4.3.1.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/6583
The text was updated successfully, but these errors were encountered: