Opened 12 years ago

Closed 12 years ago

#4667 closed enhancement (fixed)

[with patch, positive review] quadratic twists for p-adic L-functions

Reported by: wuthrich Owned by: was
Priority: minor Milestone: sage-3.4.1
Component: number theory Keywords: padic l-functions, quadratic twists
Cc: cremona, was Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mabshoff)

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.

Attachments (6)

twisted_padic_l_functions.1.patch (60.1 KB) - added by wuthrich 12 years ago.
the first part.
twisted_padic_lseries.patch (87.5 KB) - added by wuthrich 12 years ago.
New Patch, replaces the first patch. Exported against 3.2.1.
trac4667.twisted_padic_lseries.patch (90.6 KB) - added by wuthrich 12 years ago.
New patch, replaces the previous two. Exported against 3.3.rc3
trac4667_doctestfix.patch (1.1 KB) - added by GeorgSWeber 12 years ago.
apply after the last one above (it changes only one line)
trac_4667_z.patch (107.5 KB) - added by wuthrich 12 years ago.
Replaces all previous patches.
trac_4667_z2.patch (1.4 KB) - added by wuthrich 12 years ago.
apply after the previous patch

Download all attachments as: .zip

Change History (29)

Changed 12 years ago by wuthrich

the first part.

comment:1 Changed 12 years ago by wuthrich

  • Milestone changed from sage-3.3 to sage-3.2.2
  • Summary changed from [with first patch, not ready for review] quadratic twists for p-adic L-functions to [with patch, needs review] quadratic twists for p-adic L-functions

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.

Changed 12 years ago by wuthrich

New Patch, replaces the first patch. Exported against 3.2.1.

comment:2 Changed 12 years ago by GeorgSWeber

  • Summary changed from [with patch, needs review] quadratic twists for p-adic L-functions to [with patch, needs review, under review by gsw] quadratic twists for p-adic L-functions

Target time for the review: January 10th

comment:3 Changed 12 years ago by GeorgSWeber

Rescheduled for January 18th

comment:4 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, needs review, under review by gsw] quadratic twists for p-adic L-functions to [with patch, needs review] quadratic twists for p-adic L-functions

comment:5 Changed 12 years ago by GeorgSWeber

  • Summary changed from [with patch, needs review] quadratic twists for p-adic L-functions to [with patch, needs work] quadratic twists for p-adic L-functions

Needs work, but only minor issues:

applying ../../../patches/twisted_padic_lseries.patch
patching file sage/schemes/elliptic_curves/ell_rational_field.py
Hunk #5 FAILED at 4640
1 out of 5 hunks FAILED -- saving rejects to file sage/schemes/elliptic_curves/ell_rational_field.py.rej
abort: patch failed to apply

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:

sage -t -long "local/lib/python/site-packages/sage/schemes/elliptic_curves/padic_lseries.py"
**********************************************************************
File "/Users/georgweber/Public/sage/sage-3.2.3/local/lib/python/site-packages/sage/schemes/elliptic_curves/padic_lseries.py", line 644:
    sage: E.quadratic_twist(-19).label()
Exception raised:
    Traceback (most recent call last):
      File "/Users/georgweber/Public/sage/sage-3.2.3/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/Users/georgweber/Public/sage/sage-3.2.3/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/Users/georgweber/Public/sage/sage-3.2.3/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_14[22]>", line 1, in <module>
        E.quadratic_twist(-Integer(19)).label()###line 644:
    sage: E.quadratic_twist(-19).label()
      File "/Users/georgweber/Public/sage/sage-3.2.3/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 2720, in cremona_label
        raise RuntimeError, "Cremona label not known for %s."%self
    RuntimeError: Cremona label not known for Elliptic Curve defined by y^2 + y = x^3 - x^2 - 120*x - 2183 over Rational Field.
**********************************************************************
1 items had failures:
   1 of  23 in __main__.example_14
***Test Failed*** 1 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.

sage -t -long "local/lib/python/site-packages/sage/schemes/elliptic_curves/sha_tate.py"
**********************************************************************
File "/Users/georgweber/Public/sage/sage-3.2.3/local/lib/python/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 276:
    sage: EllipticCurve('123a1').sha().an_padic(41) #rank 1    (long time)
Expected:
    1 + O(41)
Got:
    40 + O(41)
**********************************************************************
File "/Users/georgweber/Public/sage/sage-3.2.3/local/lib/python/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 283:
    sage: EllipticCurve('34a1').sha().an_padic(5) # rank 0     (long time)
Exception raised:
.......
    sage: EllipticCurve('34a1').sha().an_padic(5) # rank 0     (long time)
      File "/Users/georgweber/Public/sage/sage-3.2.3/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 394, in an_padic
        lps = lp.Dp_valued_series(n,quadratic_twist=D,prec=r+1)
    TypeError: Dp_valued_series() got an unexpected keyword argument 'quadratic_twist'
........
    sage: e.sha().p_primary_bound(7)           # long time
      File "/Users/georgweber/Public/sage/sage-3.2.3/local/lib/python2.5/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 458, in p_primary_bound
        shan = self.an_padic(p,prec = 0,use_twist=True)
    TypeError: an_padic() got an unexpected keyword argument 'use_twist'

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 Changed 12 years ago by wuthrich

  • Summary changed from [with patch, needs work] quadratic twists for p-adic L-functions to [with patch, needs review] quadratic twists for p-adic L-functions

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 _is_ present. It does not belong to this trac ticket anyway.

chris.

Changed 12 years ago by wuthrich

New patch, replaces the previous two. Exported against 3.3.rc3

Changed 12 years ago by GeorgSWeber

apply after the last one above (it changes only one line)

comment:7 Changed 12 years ago by GeorgSWeber

  • Milestone changed from sage-3.4.1 to sage-3.4
  • Summary changed from [with patch, needs review] quadratic twists for p-adic L-functions to [with patch, positive review, needs rebase] quadratic twists for p-adic L-functions

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:8 Changed 12 years ago by mabshoff

  • Description modified (diff)
  • Milestone changed from sage-3.4 to sage-3.4.1

comment:9 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, positive review, needs rebase] quadratic twists for p-adic L-functions to [with patch, needs rebase] quadratic twists for p-adic L-functions

Once the rebase has been done the positive review can be added back.

Cheers,

Michael

comment:10 Changed 12 years ago by wuthrich

  • Summary changed from [with patch, needs rebase] quadratic twists for p-adic L-functions to [with patch, positive review] quadratic twists for p-adic L-functions

OK, it took me some work to rebase it. Of course the docstrings are not all restified, but this should be done in #4933.

I attach a new patch that will replace all others before. It was exported against my sage 3.4.

I added the 'positive review' back, but maybe soneone else should check the tests again, indep. of me.

Changed 12 years ago by wuthrich

Replaces all previous patches.

comment:11 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, positive review] quadratic twists for p-adic L-functions to [with patch, needs work] quadratic twists for p-adic L-functions

The latest patch causes doctest failures in two files:

	sage -t -long devel/sage/doc/en/bordeaux_2008/elliptic_curves.rst # 2 doctests failed
	sage -t -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 6 doctests failed

Cheers,

Michael

comment:12 follow-up: Changed 12 years ago by GeorgSWeber

  • Milestone changed from sage-3.4.2 to sage-3.4.1
  • Summary changed from [with patch, needs work] quadratic twists for p-adic L-functions to [with patch, positive review, 3.4.1.alpha doctest issues(?!)] quadratic twists for p-adic L-functions

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, gsw

comment:13 in reply to: ↑ 12 Changed 12 years ago by mabshoff

  • Summary changed from [with patch, positive review, 3.4.1.alpha doctest issues(?!)] quadratic twists for p-adic L-functions to [with patch, needs work] quadratic twists for p-adic L-functions

Replying to GeorgSWeber:

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. :-)

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, gsw

Cheers,

Michael

comment:14 Changed 12 years ago by GeorgSWeber

  • Milestone changed from sage-3.4.1 to sage-3.4.2

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:

File "/Users/georgweber/Public/sage/sage-3.4/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 5164:
    sage: [P[0] for P in EllipticCurve([0,0,0,-468,2592]).integral_points()]
Expected:
    [-24, -18, -14, -6, -3, 4, 6, 18, 21, 24, 36, 46, 102, 168, 186, 381, 1476, 2034, 67246]
Got:
    [-24, -18, -14, -6, -3, 4, 6, 18, 21, 24, 36, 46, 102, 186]

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, gsw

comment:15 Changed 12 years ago by wuthrich

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. It looks to me as if everything passes now. Except

File "/maths/staff/pmzcw/Desktop/sage/twists/elliptic_curves/ell_rational_field.py", line 3096:
    sage: E.cremona_label()
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: Cremona label not known for Elliptic Curve defined by y^2 + x*y + 3*y = x^3 + 2*x^2 + 4*x + 5 over Rational Field.
Got:
    '10351a1'

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.

Changed 12 years ago by wuthrich

apply after the previous patch

comment:16 Changed 12 years ago by wuthrich

I just found out that my issue with the optional package has actually a trac ticket : #5346 .

comment:17 Changed 12 years ago by mabshoff

  • Cc cremona was added
  • Summary changed from [with patch, needs work] quadratic twists for p-adic L-functions to [with patch, needs review] quadratic twists for p-adic L-functions

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 Changed 12 years ago by GeorgSWeber

There are two aspects to be considered with a patch of this kind:

  • The technical one (is the code correctly merged into the system, or are regressions introduced?). In this area of Sage I would trust the doctests, especially John has added valuable ones that should cover enough cases so regressions are caught. As was exactly the case, BTW! A reviewer has to have an eye on the new code in this regard, too --- would the new doctests show regressions possibly introduced later? (Let's say some erronuous future patch to the core code of the p-adic numbers?) I did covince myself of that two months ago, but would like to look into it again now. And alas, I seem to desparately need the oncoming two weeks Easter vacation (where I will have no Internet access at all).
  • The theoretical one (do the mathematics make sense, are the normalizations reasonable, and such?) In this regard, the patch of Chris is an absolute win for Sage. The original p-adic L-series code of William was OK, and it was good to have that code in Sage at all. But the reworkings of Chris are certainly enhancements heading in the right direction --- William?

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 Changed 12 years ago by GeorgSWeber

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 Changed 12 years ago by mabshoff

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 Changed 12 years ago by mabshoff

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 Changed 12 years ago by GeorgSWeber

  • Milestone changed from sage-3.4.2 to sage-3.4.1
  • Summary changed from [with patch, needs review] quadratic twists for p-adic L-functions to [with patch, positive review] quadratic twists for p-adic L-functions

Can't help it, this code does look good to me.

Tested against Sage-3.4.1.alpha0.

comment:23 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged trac_4667_z.patch and trac_4667_z2.patch in Sage 3.4.1.rc0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.