Opened 11 years ago

Closed 10 years ago

#10236 closed defect (fixed)

bug in modular symbols for elliptic curves

Reported by: wuthrich Owned by: cremona
Priority: major Milestone: sage-4.6.1
Component: elliptic curves Keywords: modular symbols
Cc: cremona, was Merged in: sage-4.6.1.alpha3
Authors: Chris Wuthrich Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The following two computations should yield the same answer. First with the modular symbols in sage

sage: E = EllipticCurve('11a1')
sage: m = E.modular_symbol()
sage: m(1/7)
7/10 
sage: m(0)
1/5

and then using ec_lib :

sage: m = E.modular_symbol(use_eclib=True)
sage: m(1/7)
6/5
sage: m(0)
1/5

That the actual value of [1/7] must be 7/10 is illustrated be the following

sage: ans = E.anlist(10^5)
sage: twopii = CC(2*pi*i)
sage: s = 0
sage: n = 1
sage: while n < 50000:
....:     s += ans[n]/n*exp(twopii*n/7)
....:     n += 1
sage: s.real()/E.period_lattice().basis()[0]
0.694799317284868

The fact that both values at 0 are equal show that it is unlikely that this is a problem with the scaling of the modular symbols.


Here is another bug. Maybe the same, maybe different. This one looks like being in scaling. But I am puzzled, because this example was used originally in the design of the scaling function.

sage: E = EllipticCurve('121b1')
sage: m = E.modular_symbol()
sage: m(1/7)
2
sage: m._scaling
-2

It should in fact be [1/7]+ = 1/2.

sage: ans = E.anlist(10^5)
sage: s = 0
sage: n = 1
sage: while n < 100000:
    s += ans[n]/n*exp(twopii*n/7)
    n += 1
....:     
sage: s.real()/E.period_lattice().basis()[0]
0.484665473298495

This was originally reported by Andrew Ohana.

Attachments (4)

trac_10236_1.patch (4.8 KB) - added by wuthrich 10 years ago.
This first patch solves the second issue. Exported 4.6
trac_10236_2.patch (4.9 KB) - added by wuthrich 10 years ago.
Solves the remaining problems. Exported 4.6. This patch has to be applied after the first one.
trac_10236_3.patch (2.2 KB) - added by wuthrich 10 years ago.
exported 4.6 to be applied after the two others
trac_10236_4.patch (1.2 KB) - added by wuthrich 10 years ago.
to be applied after the other three patches

Download all attachments as: .zip

Change History (18)

comment:1 Changed 11 years ago by wuthrich

One guess might be that I misinterpreted what the output of eclib is. I assumed that in line 553 in ell_modular_symbol.py that the output of

sage: from sage.libs.cremona.newforms import ECModularSymbol
sage: E = EllipticCurve('11a1')
sage: m = ECModularSymbol(E)
sage: m(1/2)
2

meant that 2 pi i \int_{r}{0} f(z) dz = m(r) * Omega where f is the modular form and Omega = 1.2692 is the real period. Maybe this is not correct. Then the documentation in sage.libs.cremona.newforms.pyx should be improved.

comment:2 Changed 11 years ago by wuthrich

The second bug is independent of the first and I found the erro in the original implementation. I will soon post a patch that covers this part of the problem.

Changed 10 years ago by wuthrich

This first patch solves the second issue. Exported 4.6

comment:3 Changed 10 years ago by wuthrich

I found all the problems now... they are all due to ME. Sorry ! I will post a patch shortly.

comment:4 Changed 10 years ago by wuthrich

On the way I found what the second patch in #9476 (changeset 14823) did. I have to revert that. It is true that eclib has now negative spaces, but the call function for negatives is not implemented in newforms.pyx. So as it was until now, E.modular_symbol(use_eclib=True, sign=-1)(r) will compute the + modular symbol. I am sorry that I did not take a look at #9476. Hence I will also revert the part of #9247 that made eclib to be chosen by default in p-adic L-functions.

I will open a separate ticket for changing this afterwards.

Changed 10 years ago by wuthrich

Solves the remaining problems. Exported 4.6. This patch has to be applied after the first one.

comment:5 Changed 10 years ago by wuthrich

  • Authors set to Chris Wuthrich
  • Status changed from new to needs_review

This second patch, applied after the first one, will solve the both issues in the ticket description and revert back some errors introduced earlier.

The problem was a "/2" in a formula. I have absolutely no idea why I put that there. To check that the formula is now correct, I did the following::

sage: rs = flatten([[a/m for a in srange(1,m) if gcd(a,m)==1] for m in srange(2,20)])
sage: for E in cremona_curves([11..100]):
....:     m = E.modular_symbol(use_eclib=False)
....:     me = E.modular_symbol(use_eclib=True)
....:     for r in rs:
....:         if m(r) != me(r):
....:             print E.label(), r, m(r), me(r)

which gladly did not yield any output.

So these patches are ready for review.

comment:6 Changed 10 years ago by wuthrich

The follow up about the negative modular symbols is at #10256.

comment:7 Changed 10 years ago by cremona

I guess that at least some of this is my fault, for not doing everything all at once at #9476.

I hope to get time to review this before too long...

comment:8 Changed 10 years ago by wuthrich

  • Status changed from needs_review to needs_work

sorry, there are docstrings to change in ell_rational_field as well. I will do that later today.

Changed 10 years ago by wuthrich

exported 4.6 to be applied after the two others

Changed 10 years ago by wuthrich

to be applied after the other three patches

comment:9 Changed 10 years ago by wuthrich

  • Status changed from needs_work to needs_review

I hope now I have found them all.

comment:10 follow-up: Changed 10 years ago by cremona

Patches look good (note: the last two are just fixing old wrong doctest output!) and apply fine to 4.6.1.alpha1. I am testing now.

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

  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

Replying to cremona:

Patches look good (note: the last two are just fixing old wrong doctest output!) and apply fine to 4.6.1.alpha1. I am testing now.

All pass!

comment:12 Changed 10 years ago by jdemeyer

Milestone is 4.6.2 (if you want this merged in 4.6.1, change the milestone accordingly)

comment:13 Changed 10 years ago by cremona

  • Milestone changed from sage-4.6.2 to sage-4.6.1

comment:14 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.