Opened 10 years ago
Closed 7 years ago
#11211 closed defect (fixed)
elliptic curve padic Lseries claims to default to eclib but doesn't
Reported by:  was  Owned by:  cremona 

Priority:  major  Milestone:  sage6.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 )
The code for making an elliptic curve padic Lseries 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)
Change History (34)
comment:1 Changed 10 years ago by
 Description modified (diff)
Changed 10 years ago by
comment:2 Changed 10 years ago by
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
 Status changed from new to needs_review
comment:4 Changed 10 years ago by
 Cc wuthruch added
comment:5 Changed 10 years ago by
 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/sage4.7.alpha4/devel/sagemain/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/sagecurrent/local/lib/python/sitepackages/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/sage4.7.alpha4/devel/sagemain/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 followup: ↓ 7 Changed 10 years ago by
 Dependencies set to #10280
 Description modified (diff)
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 10 years ago by
 Description modified (diff)
comment:8 in reply to: ↑ 7 Changed 10 years ago by
 Status changed from needs_review to needs_work
comment:9 followup: ↓ 10 Changed 10 years ago by
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
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!
comment:11 Changed 10 years ago by
 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
 Status changed from needs_review to positive_review
comment:13 Changed 10 years ago by
 Milestone changed from sage4.7 to sage4.7.1
comment:14 Changed 10 years ago by
 Status changed from positive_review to needs_work
 Work issues set to fix on 32bit
The buildbot discovered some doctest failures on 32bit 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/hawk1/hawk_full/build/sage4.7.1.alpha0/devel/sagemain/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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/sage/schemes/elliptic_curves/sha_tate.py", line 565, in an_padic lp = Et.padic_lseries(p) File "/export/home/buildbot/build/sage/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/sage/schemes/elliptic_curves/padics.py", line 159, in padic_lseries normalize = normalize, use_eclib=use_eclib) File "/export/home/buildbot/build/sage/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/devel/sagemain/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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/sage/schemes/elliptic_curves/sha_tate.py", line 565, in an_padic lp = Et.padic_lseries(p) File "/export/home/buildbot/build/sage/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/sage/schemes/elliptic_curves/padics.py", line 159, in padic_lseries normalize = normalize, use_eclib=use_eclib) File "/export/home/buildbot/build/sage/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/devel/sagemain/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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/sage/schemes/elliptic_curves/sha_tate.py", line 565, in an_padic lp = Et.padic_lseries(p) File "/export/home/buildbot/build/sage/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/sage/schemes/elliptic_curves/padics.py", line 159, in padic_lseries normalize = normalize, use_eclib=use_eclib) File "/export/home/buildbot/build/sage/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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/hawk1/hawk_full/build/sage4.7.1.alpha0/local/lib/python/sitepackages/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
 Status changed from needs_work to needs_info
I confirm the failure on 32bit computers with Sage 4.7.1, where the patch applies smoothly.
However there are already some tests that are known to fail on 32bit, 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): # 32bit ... # 32bit OverflowError: long int too large to convert to int # 32bit
Should we mark the tests which are failing on 32bit in the same way?
Paul
comment:16 Changed 7 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:17 Changed 7 years ago by
 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
 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 64bit. But I have no access to 32bit and so I would need somone to review this who has. I copied as suggested the # 32bit 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:
8a9f937  Trac 11211: eclib should be used by default for modular symbols

comment:19 Changed 7 years ago by
I should be able to test this on my 2008 laptop which is 32bit 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
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 32bit.
comment:21 Changed 7 years ago by
 Branch changed from u/wuthrich/ticket/11211 to u/cremona/ticket/11211
 Commit changed from 8a9f937ba243ac7586633a273866b76fe49d38fa to 530027c370dfc82cb2c6a989984c07f1949d5e9b
New commits:
530027c  trac 11211: correct doctest error message for 32bit

comment:22 Changed 7 years ago by
 Description modified (diff)
 Work issues fix on 32bit deleted
What the purpose of the comment
# 32bit (see :trac: `11211`)
the fact that those tests don't work on 32bit 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 32bit systems".
comment:23 Changed 7 years ago by
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.
comment:24 Changed 7 years ago by
 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
 Commit changed from 530027c370dfc82cb2c6a989984c07f1949d5e9b to 077059e75ac3bbafd2df159a5e76c6e26042f34f
Branch pushed to git repo; I updated commit sha1. New commits:
077059e  trac 11211: move sigon() and add extra tests

comment:26 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:27 Changed 7 years ago by
 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
 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
 Commit changed from 077059e75ac3bbafd2df159a5e76c6e26042f34f to 66b58a737fb33262558f4b93d887f156653ff18f
comment:30 Changed 7 years ago by
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
 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 64bit, but I understand that 32bit now passes all tests too.
Thanks for the work.
comment:32 Changed 7 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Note: there is also a call to create the Lseries in padics.py that has use_eclib=False by default and is documented correctly. The attached patch changes both cases to default to True.