-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
quadratic twists for p-adic L-functions #4667
Comments
the first part. |
comment:1
Attachment: twisted_padic_l_functions.1.patch.gz With respect to the above description, this new version of the patch does everything as said before, but the modular_symbols are handled differently. In ell_modular_symbol there are now two option, either to use or not to use eclib (for the + part at least). I implemented an optional argument that controls what method is used to normalize the modular symbols, for both eclib and sage's own modular symbols. |
New Patch, replaces the first patch. Exported against 3.2.1. |
comment:2
Attachment: twisted_padic_lseries.patch.gz Target time for the review: January 10th |
comment:3
Rescheduled for January 18th |
comment:5
Needs work, but only minor issues:
That one hunk is only an empty line replacing an empty line (?!?), so could be ignored (and wouldn't prevent a positive review from my side). But there's real work also, some doctest failures:
The doctest failure above seems to come from that I used a vanilla Sage 3.2.3, and the example seems to need the optional extended Cremona tables --- so just add a "#optional" or so. There are also a bunch of doctest failures for "sha_tate.py", which pop up (only) if the doctest "-long" option is used.
Except for the first (which might be architecture related, the old file had this doctest with a "#random" comment --- I'm using a Mac) all the following "long" doctest failures in sha_tate.py just seem to be artifacts from development, easily to be cleaned up. All in all: nothing serious, but as-is it can't go in. |
comment:6
Finally it was easier to correct than what I thought. I simply forgot to change the sign in the p-adic height for split multiplicative primes. The new patch should apply cleanly against 3.3.rc3 ( i hope ) and no test (with -long) failed except for the issue about the unknown Cremona label in line 644 that Gregor mentioned. I don't know how to solve this; the error only occurs if the optional package chris. |
New patch, replaces the previous two. Exported against 3.3.rc3 |
Attachment: trac4667.twisted_padic_lseries.patch.gz apply after the last one above (it changes only one line) |
comment:7
Attachment: trac4667_doctestfix.patch.gz As I said in my remark of #4933, this patch here definitely should be applied before any work on #4933 is done. Could you help with the rebase? It's just two of the six files where rebasing is needed, and with the scripts, shouldn't it be rather easy? P.S.: Apply the last two patches only, the latter is just a one-liner that makes a certain doctest optional. That doctest is only of illustrative nature anyway, and depends on the optional larger Cremona ell curves database for curves of conductor between 10000 and 130000. (A curve of conductor around 15000 is involved.) |
comment:12
Michael, begging your pardon, but I see none of these doctest failures against vanilla Sage-3.4 on my MacIntel box. I do confirm "positive review" (just finished testlong and docbuild)! Let's wait for 3.4.1.alpha and see, I think the reason is that some other patch that went already in your local tree causes this. It should be sorted out easily, once the first 3.4.1.alpha release is out. :-) Cheers, |
comment:13
Replying to @sagetrac-GeorgSWeber:
These failures happen against my merge tree, so no point in reinstating the positive review. I would also prefer if William took a look at this patch since he seemed to have written most of the original code.
Cheers, Michael |
comment:14
Uh oh. No offense meant, I was just too excited so I started the review after midnight. And sorry, my fault, I re-checked my setup and it seems that somehow I had forgotten a "sage -b" in the course. I do see the failures against vanilla sage-3.4, and they seem to be severe:
and most of the other failed doctests also point to missing integral points. There had been a patch of John (IIRC) that had healed an issue with this, and the current patch of Chris seems to miss these parts (more precisely seems to revert the code to the old buggy state). As a by-result, this means that I definitely shouldn't try to review patches without being well-rested. Which probably means I won't be able to do reviews at all. :-((( Cheers, |
comment:15
Uiuiui, I am sorry that was quite bad. I did not merge well ell_rational_field. I add another patch that should be applied after the previous.
but this is due to having installed the optional package. I still don't know if there is a way to include a test only if the optional package is NOT there. But anyway that is not in my code. |
Attachment: trac_4667_z2.patch.gz apply after the previous patch |
comment:16
I just found out that my issue with the optional package has actually a trac ticket : #5346 . |
comment:17
Ok, changed the summary to have the ticket picked up by the right reports again. I would also be happy if John and/or William looked at this code since it seems to touch code they have written. Cheers, Michael |
comment:18
There are two aspects to be considered with a patch of this kind:
I certainly want to make Michael's (the integrator's) life easier, not harder. And from the experience yesterday I don't trust myself right now (it's again only 10 minutes before midnight). So either the review has to wait for, say, another month or so, or a new reviewer steps in. William, John? |
comment:19
Concerning the issue with the optional database: I have added a comment to trac #5346 with an idea how to address it --- this idea applies verbatim here. Chris, you certainly are able to provide a concrete example of a (twist of a) curve such that the resulting conductor is larger than 130000, in less than a minute? Cheers, gsw |
comment:20
Hi Georg, well, all I am saying is that someone ought to take a second look that nothing got broken that used to work before and that we do not notice because the doctest was "corrected". I am not claiming that this is the case, but I want to be 100% on this. I personally have no clue about padic l-functions and do not know who the experts are in the field, so I wanted to err on the side of caution by pinging John and William. I did mention this patch to William while he was sitting next to me yesterday and hopefully he will have time to look at this during the weekend - most likely sunday, but we will see what happens. It would be very nice to get this into 3.4.1 :) I will certainly doctest the two current patches and give feedback. Cheers, Michael |
comment:21
FYI: if I apply trac_4667_z.patch and trac_4667_z2.patch to my 3.4.1.alpha0 merge tree all tests pass. Cheers, Michael |
comment:22
Can't help it, this code does look good to me. Tested against Sage-3.4.1.alpha0. |
comment:23
Merged trac_4667_z.patch and trac_4667_z2.patch in Sage 3.4.1.rc0. Cheers, Michael |
This ticket is aimed at implementing p-adic L-function of quadratic twists of elliptic curves.
Of course one could compute the modular symbols for the twist and then the p-adic L-function, but it is much faster to use the simple formula for the twisted modular symbols as a sum of
modular symbols. See section 8 in Mazur-Tate-Teitelbaum = [MTT].
Here is a list of things implemented and changed by this patch. Maybe this is too long and it
would be preferable to split it up into smaller patches. I will add more documentation and more compatibility checks in the docstring.
ell_modular_symbol.py
I changed completely the normalization. Until now, the normalization was done in such a way as to guarantee that [0]+, the modular_symbol(0) for sign=+1, is equal to L(E_0,1)/Omega_E_0, where E_0 is the optimal X_0(N) curve in the isogeny class of E and Omega_* is the least positive real period of E. Up to sign, which was arbitrary, and up to a factor 2, which comes from the fact that we do not know that the Manin-constant is 1 as it should be. (I hope I got this right).
Now instead I normalize it in such a way as to make sure that [0]+ is equal to L(E,1)/Omega_E. So the result will depend on the curve E and not only on the isogeny class. Let {0}+ be the non-normalized modular symbol. If the above algebraic L-value is non-zero, then so is {0}+ and they are rational multiples of each other with small numerator and denominator. So we just compute a real approximation to this fraction and we scale {0}+ to get [0]+. If the L-value is zero, then we try to use a quadratic twist of E by a small positive (or later negative) fundamental discriminant, hoping that we will hit one that is not zero. If we fail to find one, we have to go back to the previous way of normalizing, but multiplying with Omega_E_0/Omega_E as to make sure that the missed factor +-1/2 is the same for all curves in the isogeny class. And we issue a warning to the user.
Right now, I substituted the normalization by mine, it would be easy to change the optional argument in such a way as to allow the user to choose between the two normalizations.
I have tested the correctness of this for hundreds of curves, using twists by -31 and 37 that do
not appear in the code. Of course, there will be curves for which the new normalization fails. For negative modular symbols we only use negative twists (in order to avoid an obvious infinite loop). But we are safe to assume that negative modular symbols are not very often used without the positive ones.
I have not rewritten the normalization of modular symbols coming from ec_lib in padic_lseries.py. This should be done, but I have no idea if the negative symbols can be computed with that library.
Added more docstrings.
padic_lseries.py
I got rid of quotient_of_period which is no longer needed because of the above normalization.
I added a try around the cremona-label look-up. Caused a bug before. modular_symbol has now an optional argument to twist by a fundamental discriminant D. Similar for measure and series.
New we need a function to compute the quotient of Omega_E by Omega(ED)*sqrt(D), if D>0, or Omega-E by Omega(ED)*sqrt(-D), if D<0. According to [MTT] this should be 1 or 2. This
is done in _quotient_of_periods_to_twist.
Furthermore, I have changed the sign of the Dp-values height, just like I had to change the sign in the canonical p-adic height in the ordinary case.
So far I have tested this on good ordinary primes for curves of rank 0 and rank 1 and some rank 2 curves. I have also checked a few supersingular cases.
padics.py
I changed the sign of the padic height. It is now clear that there must be a -1 in front to make sure that the p-adic BSD conjecture holds as stated in [MTT].
I also removed the statement that the equation must be globally minimal to compute the height as the gens() no longer fails for non-minimal curves.
ell_rational_field.py
adjusted the documentation according to the change in ell_modular_symbol.
added a function minimal_quadratic_twist that find a twist of E with minimal conductor. This is used in sha_tate.py but could be of use later in rank as well.
todo : normalization if using ec_lib (?)
todo : add in rank() the possibility to use modular-symbols for a twisted curve with low conductor
sha_tate.py
an_padic has now an optional argument to control whether the modular symbols computations can be done on a minimal quadratic twist instead.
CC: @JohnCremona @williamstein
Component: number theory
Keywords: padic l-functions, quadratic twists
Issue created by migration from https://trac.sagemath.org/ticket/4667
The text was updated successfully, but these errors were encountered: