-
-
Notifications
You must be signed in to change notification settings - Fork 528
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
Extend coleman integration to handle Weierstrass points #7927
Comments
some code, probably needs to be cleaned up |
Attachment: 7927-coleman-weierstrass.patch.gz Attachment: 13535.patch.gz more Weierstrass code |
comment:1
Here's a first attempt at some more code, with a little bit of overlap with Robert's code. I like his incorporation of the non-Teichmuller algorithm as an optional parameter in the main Coleman integral function, so we can mostly ignore what I do instead. The main functions of interest are The auxiliary functions (e.g., P_to_S, S_to_Q, etc.) aren't likely to be of much interest to the user, but they give us good consistency checks. I'd appreciate input on renaming/restructuring things. And my apologies for the messed-up line breaks in hyperelliptic_generic -- just noticed them now, but boxen.math (where I've been editing) has been difficult to access in the last hour from Guam. I can fix this (my) tomorrow morning. |
Attachment: 13536.patch.gz apply after 13535 to fix line break problems |
comment:2
The overlap needs to be taken care of somehow. It might be easiest for Jen to incorporate whatever is appropriate from Robert's patch. I'm dubious about the treatment of points in the infinite disc, on several counts. One is whether the Frobenius gets the y-coordinate right, since I think in both patches the check passes for trivial reasons whether or not the y-coordinate is right. Under Robert's patch, one gets lucky: you win as long as the square root of a p-adic number with unit part congruent to 1 mod p is guaranteed to be congruent to 1 mod p. This is undocumented but appears to be true. Under Jen's patch, one does not get lucky:
More seriously, computing Coleman integrals even between two points in the infinite disc seems to be broken. Under Robert's patch, we have:
The last two lines should be the same; right now, they aren't even of the same return type (one is a list, one is a tuple). Under Jen's patch, the last call returns an error instead:
|
comment:3
Followup: the p-adic square root is computed using pari, which always chooses the branch of the square root so that the leading p-adic digit is in [0, p/2]. So Robert's treatment of the ambiguity does seem to work. |
merged Robert's patch with mine, though still a work in progress |
Attachment: 13537.patch.gz Attachment: 13538.patch.gz fixing precision issues in the infinite disc |
comment:4
The good news is, this looks fine on mathematical grounds. However, there are a few procedural issues before I can issue a positive review. I am getting some doctest failures in hyperelliptic_padic_field.py. I think the problem is the indentation in the doctest for is_same_disc. If I fix that by hand, the doctests all pass. In other news, coverage checking points up some missing documentation/doctests.
For hyperelliptic_generic.py:
Of these, the ones that are new (is_in_weierstrass_disc, is_weierstrass, and maybe some others) absolutely need doctests. Improving the coverage for older functions would be helpful but is not an obstacle to a positive review on this ticket (we could create a separate ticket for that). Another minor note: various release managers have requested that the commit line for patches start with the number of the relevant ticket, e.g., #7927: Extend coleman integration to handle Weierstrass points This is helpful in case a rollback is needed after applying a whole bunch of disparate patches. This comment can be ignored unless you decide to build a single patch encompassing all of the changes so far. |
comment:5
It appears I spoke to soon. An example which used to work but is now broken:
|
comment:6
Replying to @kedlaya:
... This failure occurred using 4.2.1. I can't reproduce it using 4.3.3.alpha0, so it may be an artifact. |
fixing things in comment #4 (doctests, etc.) |
Attachment: 13539.patch.gz doctesting exceptions; formatting of docstrings |
comment:7
Attachment: 13540.patch.gz |
removed coleman_integrals_on_basis in ell_padic_field.py |
comment:8
Attachment: 13541.patch.gz After applying 13535.patch through 13541.patch against 4.3.3.alpha0, I get no long doctest failures anywhere in sage/schemes/. Positive review. For other issues with Coleman integration, see tickets #8304 and #8305. |
Merged: sage-4.3.4.alpha0 |
comment:9
Merged in this order: Jennifer: you should put the ticket number in your patch. |
Author: Jennifer Balakrishnan, Robert Bradshaw |
Reviewer: Kiran Kedlaya |
comment:10
What's up with this condition:
I totally cannot understand why you check whether the variable name is |
comment:11
See #24267 for a follow-up. |
CC: @jbalakrishnan
Component: number theory
Author: Jennifer Balakrishnan, Robert Bradshaw
Reviewer: Kiran Kedlaya
Merged: sage-4.3.4.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/7927
The text was updated successfully, but these errors were encountered: