-
-
Notifications
You must be signed in to change notification settings - Fork 481
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
refactor heegner points code out of ell_rational_field and support computing higher heegner points #6616
Comments
Attachment: 6616-higher-heegner.patch.gz |
This comment has been minimized.
This comment has been minimized.
comment:2
Attachment: trac_6616-part3.patch.gz Next (?):
|
comment:3
This calculation uses massive RAM:
This should get dealt with. No clue what causes this. |
Attachment: trac_6616-part5.patch.gz |
Attachment: trac_6616-part6.patch.gz |
This comment has been minimized.
This comment has been minimized.
rebased against 4.1.1 |
This comment has been minimized.
This comment has been minimized.
comment:5
Attachment: trac_6616-part2.patch.gz |
rebased against 4.1.1 |
Attachment: trac_6616-part4.patch.gz rebased against 4.1.1 |
comment:6
Attachment: trac_6616-part7.patch.gz |
This comment has been minimized.
This comment has been minimized.
Attachment: trac_6616-part8.patch.gz |
This comment has been minimized.
This comment has been minimized.
comment:8
Attachment: trac_6616-part10.patch.gz |
This comment has been minimized.
This comment has been minimized.
Attachment: trac_6616-part11.patch.gz |
Attachment: trac_6616-part12.patch.gz |
Attachment: trac_6616-patch13.patch.gz |
This comment has been minimized.
This comment has been minimized.
Attachment: trac_6616-part14.patch.gz |
comment:11
Attachment: trac_6616-part9.patch.gz |
This comment has been minimized.
This comment has been minimized.
clean bundle with 1-16 against sage-4.1.1 |
comment:13
Attachment: heegner-1-to-16.hg.gz This is a clean bundle against sage-4.2.alpha0: http://sage.math.washington.edu/home/wstein/patches/heegner-20091021-rebased_against_4.2.alpha0.hg |
Attachment: heegner-4.2.1.hg.gz rebased against 4.2.1 |
this is based against 4.2.1 after applying the hg bundle right above this patch. |
Attachment: trac_6616_part17.patch.gz Attachment: trac_6616-part18.patch.gz |
Attachment: trac-6616-rebase_complete-4.3.hg.gz Attachment: trac-6616-everything_rebased_against_4.3.patch.gz apply just this one patch against a clean sage-4.3 |
comment:14
This is finally ready for review. Apply just this patch against a clean sage-4.3 build:
|
comment:16
Review: I am immensely impressed by the huge amount of work which has gone into this ten thousand-line patch! It goes far beyond the refactoring of the title. I read through the patch itself, and liked what I saw. (There are a few minor typos in the docstrings, which I did not note as I went through -- sorry). Some docstrings which did not format properly: numerical_approx, rational_kolyvagin_divisor, KolyvaginPoint. There are a lot if interesting mathematical and algorithmic issues which are highlighted very well in comments and TODO blocks, which is good. Testing all files in the elliptic_curves directory I found these problems.
In heegner.py it's mainly a mixture of numerical noise and hash differences (this is on a 32-bit machine), but not all. Details:
Incidentally that test takes 90s which is rather long (I have not tested it with "-long" yet!) Some of this might be caused by the fact that I have a new version of lcalc (from reviewing another ticket) although this is a fresh clone?
The perennial noise issues in period_lattice.py:
Lastly, in ell_rational_field.py, we have some more of the same plus a very weird issue with "rubenstein":
So the conclusion is, regretfully, "needs work", though the work that needs doing is trivial compared with what has been done already (which is probably worth a PhD thesis for someone!). |
Reviewer: John Cremona |
Attachment: trac-6616-everything_rebased_against_4.3-part2.patch.gz apply this and the previous patch; then this should past all tests on 32-bit as well as 64-bit. |
comment:18
I posted a new patch fixing all the issues above, except possibly the "very weird" ell_rational_field issue. I couldn't replicate that on any test machine yet. I also didn't carefully reformat the docstrings. Which ones aren't formatted correctly? I thought I had got them right. |
Attachment: trac-6616-everything_rebased_against_4.3-part2.2.patch.gz apply instead of previous one |
comment:19
cremona>> The patches say they are based on 4.3 rather than 4.3.1. the first
was> That's in a doctest for something probably unrelated, so can be safely
Ignoring that, there was just one small thing (testing on 64-bit), namely
which is just because someone forgot to add the tag # 32-bit. I added that by editing the patch, and the corrected version is now attached here. So: positive review on 64-bit. I'll test on 32-bit when I get home (leaving now, hope I do not get buried in the snow...) |
Apply after previous (reviewer's patch fixing some docstring issues) |
comment:20
Attachment: trac-6616-review.patch.gz Positive review after testing on 32-bit, and also fixing lots of documentation glitches (in the reviewer's patch). These were mainly found by actually running "sage -docbuild all html" and reading the error messages (hint hint). Most common error was double :: where they shouldn't be. Great work! |
Author: William Stein |
Merged: sage-4.3.1.rc0 |
comment:21
Applied:
|
I made a clean hg bundle against sage-4.1.1 that includes patches 1-16 along with the patches from 6745 and 6751:
http://sage.math.washington.edu/home/wstein/patches/heegner-1-to-16.hg
NOTE: If you apply patches, the code after part 7 depends on #6745, #6751, so make sure you apply those patches!
PLAN:
Refactor ell_rational_field, so the Heegner points code (nearly 1000 lines!) is in a new file heegner.py.
Extend the heegner point code to compute y_lambda for lambda > 1.
Implement first ever algorithm for provable verification of Kolyvagin's conjecture for specific rank 2 curves.
I'm doing 1 and 2 together, since 2 is a fairly small change, but to do it right and make it at all fast requires restructuring the code a lot, otherwise one ends up fighting a lot with badly structured code instead of focusing on optimizing algorithms.
Also, this refactoring is a long time coming!
CC: @williamstein @JohnCremona
Component: number theory
Author: William Stein
Reviewer: John Cremona
Merged: sage-4.3.1.rc0
Issue created by migration from https://trac.sagemath.org/ticket/6616
The text was updated successfully, but these errors were encountered: