Opened 10 years ago

Closed 7 years ago

#11211 closed defect (fixed)

elliptic curve p-adic L-series claims to default to eclib but doesn't

Reported by: was Owned by: cremona
Priority: major Milestone: sage-6.1
Component: elliptic curves Keywords:
Cc: wuthrich Merged in:
Authors: William Stein, John Cremona Reviewers: John Cremona, Jeroen Demeyer, Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/ticket/11211 (Commits) Commit: 66b58a737fb33262558f4b93d887f156653ff18f
Dependencies: #10280 Stopgaps:

Description (last modified by jdemeyer)

The code for making an elliptic curve p-adic L-series in schemes/elliptic_curves/padic_lseries.py starts like this:

    def __init__(self, E, p, use_eclib=False, normalize='L_ratio'):
        r"""
        INPUT:

        -  ``E`` - an elliptic curve
        -  ``p`` - a prime of good reduction
        -  ``use_eclib`` - bool (default:True); whether or not to use
           John Cremona's ``eclib`` for the computation of modular
           symbols

Note that claim that use_eclib defaults to True and the fact that it doesn't.

Since eclib is supposed to be correct, and is much faster, it should be the default as claimed in the docstring. The fix should be a patch that changes the one __init__ line.

Attachments (2)

trac_11211.patch (2.2 KB) - added by was 10 years ago.
trac_11211-modsym.patch (3.8 KB) - added by cremona 10 years ago.
Replaces previous; applies to 4.7.alpha5

Download all attachments as: .zip

Change History (34)

comment:1 Changed 10 years ago by was

  • Description modified (diff)

Note: there is also a call to create the L-series in padics.py that has use_eclib=False by default and is documented correctly. The attached patch changes both cases to default to True.

Changed 10 years ago by was

comment:2 Changed 10 years ago by cremona

I have CC'd Chris Wuthrich who I think is responsible for this code and should be able to judge. It may be that there is some problem with part of eclib's modular symbols so they don't give exactly what is needed for this application.

comment:3 Changed 10 years ago by was

  • Status changed from new to needs_review

comment:4 Changed 10 years ago by cremona

  • Cc wuthruch added

comment:5 Changed 10 years ago by cremona

  • Authors set to William Stein
  • Reviewers set to John Cremona

I get zillions of errors in padic_lseries.py with this patch. (31 in fact).

They all seem to be either

File "/home/jec/sage-4.7.alpha4/devel/sage-main/sage/schemes/elliptic_curves/padics.py", iled example:
    sig_on_count()
Expected:
    0
Got:
    1

or

        def __reduce__(self):
      File "rational.pyx", line 484, in sage.rings.rational.Rational.__set_value (sage/rings/rational.c:7320)
        elif isinstance(x, tuple) and len(x) == 2:
      File "/home/jec/sage-current/local/lib/python/site-packages/sage/modular/cusps.py", line 512, in _rational_
        raise TypeError, "cusp %s is not a rational number"%self
    TypeError: cusp Infinity is not a rational number

Also in padics.py I get similar sig_on_count errors, and also

File "/home/jec/sage-4.7.alpha4/devel/sage-main/sage/schemes/elliptic_curves/padics.py", line 138:
    sage: P(0)
Expected:
    3 + 3^2 + 2*3^4 + 2*3^5 + O(3^6)
Got:
    2 + 3 + 3^2 + 2*3^3 + 2*3^5 + 3^6 + O(3^7)

which looks not so trivial.

I'll leave this as "needs review" and hope that Chris will also take a look.

comment:6 follow-up: Changed 10 years ago by mderickx

  • Dependencies set to #10280
  • Description modified (diff)

comment:7 in reply to: ↑ 6 ; follow-up: Changed 10 years ago by cremona

  • Description modified (diff)

Replying to mderickx: OK, I'm testing again against 4.7.alpha5 which has #10280 merged.

comment:8 in reply to: ↑ 7 Changed 10 years ago by cremona

  • Status changed from needs_review to needs_work

Replying to cremona:

Replying to mderickx: OK, I'm testing again against 4.7.alpha5 which has #10280 merged.

I still get lots of failures (they look just the same) in the same two files. I don't know what is going on.

comment:9 follow-up: Changed 10 years ago by mderickx

Hej John,

Sorry to make you think that the dependency would fix the issue. I just searched for trac to get a ticket which had the text "depends on" so I could test the new dependencies box I just created (look at the diff, it already said it depended on 10280).

comment:10 in reply to: ↑ 9 Changed 10 years ago by cremona

Replying to mderickx:

Hej John,

Sorry to make you think that the dependency would fix the issue. I just searched for trac to get a ticket which had the text "depends on" so I could test the new dependencies box I just created (look at the diff, it already said it depended on 10280).

No problem, I was just being optimistic. I'm sure I can work out what is going on, since the problems arise only when eclib is used!

Changed 10 years ago by cremona

Replaces previous; applies to 4.7.alpha5

comment:11 Changed 10 years ago by cremona

  • Authors changed from William Stein to William Stein, John Cremona
  • Cc wuthrich added; wuthruch removed
  • Description modified (diff)
  • Status changed from needs_work to needs_review

The new patch (replaces previous) now works: there was some integer overflow.

Now I cannot be the one to give this a positive review, though it deserves it! William? Chris?

comment:12 Changed 10 years ago by was

  • Status changed from needs_review to positive_review

comment:13 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1

comment:14 Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to fix on 32-bit

The buildbot discovered some doctest failures on 32-bit systems which I believe to come from this ticket (but I'm not completely sure):

sage -t -long  -force_lib devel/sage/sage/schemes/elliptic_curves/sha_tate.py
**********************************************************************
File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/devel/sage-main/sage/schemes/elliptic_curves/sha_tate.py", line 483:
    sage: EllipticCurve('858k2').sha().an_padic(7)   # rank 0, non trivial sha, long time (10s on sage.math, 2011)
Exception raised:
    Traceback (most recent call last):
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_7[5]>", line 1, in <module>
        EllipticCurve('858k2').sha().an_padic(Integer(7))   # rank 0, non trivial sha, long time (10s on sage.math, 2011)###line 483:
    sage: EllipticCurve('858k2').sha().an_padic(7)   # rank 0, non trivial sha, long time (10s on sage.math, 2011)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 565, in an_padic
        lp = Et.padic_lseries(p)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 159, in padic_lseries
        normalize = normalize, use_eclib=use_eclib)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/padic_lseries.py", line 199, in __init__
        self._modular_symbol = E.modular_symbol(sign=+1, use_eclib = use_eclib, normalize=normalize)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 1221, in modular_symbol
        M = ell_modular_symbols.ModularSymbolECLIB(self, sign, normalize=normalize)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_modular_symbols.py", line 499, in __init__
        self._modsym = ECModularSymbol(E)
      File "newforms.pyx", line 70, in sage.libs.cremona.newforms.ECModularSymbol.__init__ (sage/libs/cremona/newforms.cpp:1825)
    OverflowError: long int too large to convert to int
**********************************************************************
File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/devel/sage-main/sage/schemes/elliptic_curves/sha_tate.py", line 740:
    sage: e.sha().p_primary_bound(3)  # long time (10s on sage.math, 2011)
Exception raised:
    Traceback (most recent call last):
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_8[13]>", line 1, in <module>
        e.sha().p_primary_bound(Integer(3))  # long time (10s on sage.math, 2011)###line 740:
    sage: e.sha().p_primary_bound(3)  # long time (10s on sage.math, 2011)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 766, in p_primary_bound
        shan = self.an_padic(p,prec = 0,use_twists=True)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 565, in an_padic
        lp = Et.padic_lseries(p)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 159, in padic_lseries
        normalize = normalize, use_eclib=use_eclib)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/padic_lseries.py", line 199, in __init__
        self._modular_symbol = E.modular_symbol(sign=+1, use_eclib = use_eclib, normalize=normalize)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 1221, in modular_symbol
        M = ell_modular_symbols.ModularSymbolECLIB(self, sign, normalize=normalize)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_modular_symbols.py", line 499, in __init__
        self._modsym = ECModularSymbol(E)
      File "newforms.pyx", line 70, in sage.libs.cremona.newforms.ECModularSymbol.__init__ (sage/libs/cremona/newforms.cpp:1825)
    OverflowError: long int too large to convert to int
**********************************************************************
File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/devel/sage-main/sage/schemes/elliptic_curves/sha_tate.py", line 748:
    sage: e.sha().an_padic(7)  # long time (depends on "e.sha().p_primary_bound(3)" above)
Exception raised:
    Traceback (most recent call last):
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_8[15]>", line 1, in <module>
        e.sha().an_padic(Integer(7))  # long time (depends on "e.sha().p_primary_bound(3)" above)###line 748:
    sage: e.sha().an_padic(7)  # long time (depends on "e.sha().p_primary_bound(3)" above)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/sha_tate.py", line 565, in an_padic
        lp = Et.padic_lseries(p)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/padics.py", line 159, in padic_lseries
        normalize = normalize, use_eclib=use_eclib)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/padic_lseries.py", line 199, in __init__
        self._modular_symbol = E.modular_symbol(sign=+1, use_eclib = use_eclib, normalize=normalize)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 1221, in modular_symbol
        M = ell_modular_symbols.ModularSymbolECLIB(self, sign, normalize=normalize)
      File "/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_modular_symbols.py", line 499, in __init__
        self._modsym = ECModularSymbol(E)
      File "newforms.pyx", line 70, in sage.libs.cremona.newforms.ECModularSymbol.__init__ (sage/libs/cremona/newforms.cpp:1825)
    OverflowError: long int too large to convert to int
**********************************************************************

comment:15 Changed 9 years ago by zimmerma

  • Status changed from needs_work to needs_info

I confirm the failure on 32-bit computers with Sage 4.7.1, where the patch applies smoothly.

However there are already some tests that are known to fail on 32-bit, for example in lib/cremona/newforms.pyx:

            sage: from sage.libs.cremona.newforms import ECModularSymbol
            sage: E = EllipticCurve('858k2')
            sage: ECModularSymbol(E)
            Traceback (most recent call last):                     # 32-bit
            ...                                                    # 32-bit
            OverflowError: long int too large to convert to int    # 32-bit

Should we mark the tests which are failing on 32-bit in the same way?

Paul

comment:16 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:17 Changed 7 years ago by wuthrich

  • Branch set to u/wuthrich/ticket/11211
  • Modified changed from 08/13/13 15:35:53 to 08/13/13 15:35:53

comment:18 Changed 7 years ago by wuthrich

  • Commit set to 8a9f937ba243ac7586633a273866b76fe49d38fa
  • Status changed from needs_info to needs_review

I have tried to rebase this ticket, too. It seems to pass all test on my 64-bit. But I have no access to 32-bit and so I would need somone to review this who has. I copied as suggested the # 32-bit into sha_tate at the three occasions listed above. But I have no idea if that is all that is needed for 32 bit.

Sorry, if I did something wrong here.


New commits:

8a9f937Trac 11211: eclib should be used by default for modular symbols

comment:19 Changed 7 years ago by cremona

I should be able to test this on my 2008 laptop which is 32-bit and on which I built 6.0 yesterday. If it does, then Chris or Jeroen can give it a positive review?

comment:20 Changed 7 years ago by cremona

OK, so I had to change 3 error messages from

OverflowError: long int too large to convert to int

to

OverflowError: Python int too large to convert to C long

and then tests pass on 32-bit.

comment:21 Changed 7 years ago by cremona

  • Branch changed from u/wuthrich/ticket/11211 to u/cremona/ticket/11211
  • Commit changed from 8a9f937ba243ac7586633a273866b76fe49d38fa to 530027c370dfc82cb2c6a989984c07f1949d5e9b

New commits:

530027ctrac 11211: correct doctest error message for 32-bit

comment:22 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Work issues fix on 32-bit deleted

What the purpose of the comment

# 32-bit (see :trac: `11211`)

the fact that those tests don't work on 32-bit has nothing to do with this ticket, right? I would expect that ticket number to refer to a ticket saying "make sure these tests pass even on 32-bit systems".

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:23 Changed 7 years ago by jdemeyer

One other small but important comment (possibly for a different ticket, but I felt I should mention it): do not put Python code inside sig_on(), espcially not if that Python code can raise exceptions. In particular, the following 3 lines (possibly more) should be outside the sig_on() block:

r = Cusp(r) 
d = r.denominator() 
n = r.numerator() 

The consequences of this can be severe:

sage: from sage.libs.cremona.newforms import ECModularSymbol
sage: E = EllipticCurve('11a')
sage: M = ECModularSymbol(E)
sage: M("garbage")
...
TypeError: unable to convert garbage to a rational
sage: pari("1/0")
...
------------------------------------------------------------------------
Unhandled SIGSEGV: A segmentation fault occurred in Sage.
This probably occurred because a *compiled* component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Sage will now terminate.
------------------------------------------------------------------------

I also recommend that the line M("garbage") should become a doctest.

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:24 Changed 7 years ago by cremona

  • Reviewers changed from John Cremona to John Cremona, Jeroen Demeyer
  • Status changed from needs_review to needs_work

I will move the sigon() and upload a new commit tomorrow, with some new doctests.

comment:25 Changed 7 years ago by git

  • Commit changed from 530027c370dfc82cb2c6a989984c07f1949d5e9b to 077059e75ac3bbafd2df159a5e76c6e26042f34f

Branch pushed to git repo; I updated commit sha1. New commits:

077059etrac 11211: move sigon() and add extra tests

comment:26 Changed 7 years ago by cremona

  • Status changed from needs_work to needs_review

comment:27 Changed 7 years ago by jdemeyer

  • Reviewers changed from John Cremona, Jeroen Demeyer to John Cremona, Jeroen Demeyer, Chris Wuthrich

If you accept this commit, positive_review to the sig_on() stuff (but I hope somebody else can review the rest of the ticket).

comment:28 Changed 7 years ago by jdemeyer

  • Branch changed from u/cremona/ticket/11211 to u/jdemeyer/ticket/11211
  • Modified changed from 01/03/14 20:54:20 to 01/03/14 20:54:20

comment:29 Changed 7 years ago by jdemeyer

  • Commit changed from 077059e75ac3bbafd2df159a5e76c6e26042f34f to 66b58a737fb33262558f4b93d887f156653ff18f

New commits:

66b58a7Move Rational construction outside sig_on()/sig_off()
14549ceMerge branch 'develop' into ticket/11211

comment:30 Changed 7 years ago by cremona

Chris, can you give this a positive review now? You are the only reviewer not also an author, I think.

comment:31 Changed 7 years ago by wuthrich

  • Status changed from needs_review to positive_review

The tests pass for me (except a couple which I know is related to relocating the directory and not to this ticket). I can only test 64-bit, but I understand that 32-bit now passes all tests too.

Thanks for the work.

comment:32 Changed 7 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.