Opened 9 years ago

Closed 3 years ago

#10256 closed enhancement (fixed)

make -1 modular symbols from eclib available to elliptic curves

Reported by: wuthrich Owned by: cremona
Priority: major Milestone: sage-7.5
Component: elliptic curves Keywords: modular symbols
Cc: cremona Merged in:
Authors: John Cremona Reviewers: Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: 49be7d8 (Commits) Commit: 49be7d89ed221986f0d3f6df7debf401f1694a3a
Dependencies: #22077 Stopgaps:

Description

This ticket is a follw-up from #10236. In there certain changes from #9476 and #9247 were reverted back. Here is what has to be done to make modular symbols with negative sign available for computations with elliptic curves, e.g. p-adic L-series.

Currently eclib can compute the space of modular symbols of sign = -1, but ECModularSymbol in sage.libs.cremona.newforms.pyx can not use them yet to compute {0,r}-

Once this is there, one can modify sage.schemes.elliptic_curves.ell_modular_symbols.py to allow sign = -1 in ModularSymbolECLIB. (Where one has to think a little bit about the normalization.) This would allow to switch in line 1098 of ell_rational_field.py and in line 131 of padic_lseries.py to make use_eclib=True to be default.

Change History (33)

comment:1 Changed 9 years ago by cremona

The current eclib (eclib-20100711.spkg) has normalization problems in the "sign=-1" modular symbols, which I am currently working on fixing. The Sage interface should perhaps wait until that is done.

comment:2 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:3 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:4 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:6 Changed 3 years ago by wuthrich

Ping ?

I was working on #20864 when I changed the plus symbols to be computed by eclib by default. I wondered if the negative could be added now? Sage normalises the negative ones correctly now. We only need some rational multiple from eclib.

comment:7 Changed 3 years ago by cremona

Implementing this just went up my to-do list, after hearing a lecture by Mazur on his experimental work with Rubin which makes heavy use of this modular symbol functionality. Karl told me that it would save them a lot of time for minus symbols to be available from eclib. As soon as I can, I will.

Last edited 3 years ago by cremona (previous) (diff)

comment:8 Changed 3 years ago by wuthrich

Excellent. I will help of course.

I am curious what Rubin and Mazur are up to, too.

comment:9 Changed 3 years ago by cremona

I will make this ticket dependent on another which will upgrade the eclib version in Sage to one which will be released later today. Two reasons: (1) I fixed a bug which sometimes caused the plus symbols to all be mutiplied by a factor >1 (in known examples, a factor of 2). (2) I have added a facility to eclib to base symbols at infinity (oo) instead of 0, i.e. it computes the image of either {0,r} or {oo,r} for a given rational.

Example:

sage: E=EllipticCurve("121b1")
sage: M=E.modular_symbol(1)
Warning : Could not normalize the modular symbols, maybe all further results will be multiplied by -1, 2 or -2.
sage: M(1/7)
-2

Here the error message is coming from Sage, not eclib. And the correct value is 1 though the current eclib version returns 2.

comment:10 Changed 3 years ago by cremona

See #22077

comment:11 Changed 3 years ago by cremona

  • Authors set to John Cremona
  • Dependencies set to #22077

#22077 is now ready for review. I will base by branch for this ticket on that one, so am now setting that to be a prerequisite to this.

comment:12 Changed 3 years ago by cremona

  • Branch set to u/cremona/10256
  • Commit set to 6637227842765bdf48022f7c7025d3bcb310b323
  • Milestone changed from sage-6.4 to sage-7.5
  • Status changed from new to needs_review

I have set this to Needs Review but note that #22077 (upgrading eclib to version v20161230) is a dependency and is currently also waiting review.

comment:13 Changed 3 years ago by wuthrich

  • Status changed from needs_review to needs_work

I am working on this. I spotted a few misprints, but more seriously:

  • This code also changes the normalization for modular symbols in sage. In particular, the warning for 121b1 is no longer there, despite the function returning the wrong result.
    sage: E = EllipticCurve("121b1")
    sage: m = E.modular_symbol(implementation="sage")
    sage: m(1/7)
    -1/2
    sage: me = E.modular_symbol()
    sage: me(1/7)
    1/2
    
  • The second thing is that there are more places which need to change taking eclib automatically for negative symbols. Like line 219 in padic_lseries.py

comment:14 Changed 3 years ago by wuthrich

  • Branch changed from u/cremona/10256 to u/wuthrich/ticket/10256
  • Commit changed from 6637227842765bdf48022f7c7025d3bcb310b323 to 724a20522ca193a859f85dfe275bb876613edccd
  • Reviewers set to Chris Wuthrich

I found that eclib returns incorrect values currently. This is what causes all the test failures in sage.modular.pollack_stevens after my additional changes. Here is the example I am certain about:

sage: E = EllipticCurve('11a')
sage: m = E.modular_symbol(-1,implementation="eclib")
sage: ms = E.modular_symbol(-1,implementation="sage")
sage: m(-1/3)
1/2
sage: ms(-1/3)
-1/2
sage: la = sum(E.an(n)/n*exp(2*RR.pi()*I*(-1/3)*n) for n in [1..100000]); la
-0.387171140749021 - 1.47418172900132*I
sage: E.period_lattice().basis()
(1.26920930427955, 0.634604652139777 + 1.45881661693850*I)

This shows that sage's normalisation is correct and eclib has missed the sign.


New commits:

1b75fccMerge branch 'u/cremona/22077' of git://trac.sagemath.org/sage into eclib
4f51d79Merge branch 'u/cremona/10256' of git://trac.sagemath.org/sage into eclib
dcacecetrac #10256: reviewer patch first part
724a205trac #10256: highlight wrong values

comment:15 Changed 3 years ago by wuthrich

At this stage, it goes back to John to figure out - if he wants - how to get the correct sign already in eclib.

In the long term, once #21046 is in, I can use the numerical ones to do a very quick and efficient scaling. So one could also wait.

The last modification I did was to highlight the above value in the doctest. There might be others that are wrong.

comment:16 Changed 3 years ago by cremona

I am looking at this right now to see if it is really eclib and not the interface, Back soon...

comment:17 Changed 3 years ago by cremona

When I run eclib's own program (called ecnf) on this curve [0,-1,1,-10,-20] I find that {oo, 1/3} and {oo, -1/3} both map to (-3/5,-1), meaning that the plus symbol is -3/5 and the minus symbol is in both cases -1. This must be a bug since (by definition) theminus symbol should have been negated.

Apologies. This will require yet another fix to eclib, hence to #22077. I'll work on that right away.

comment:18 Changed 3 years ago by cremona

The previous comment needs correcting since I was testing on the wrong machine and not actually useing the latest eclib version v20161230. Now eclib's ecnf program maps {oo,-1/3} to (-3/5,+1) as it should for consistency. But consistency is not correctness, and in fact eclib only normalizes (w.r.t. sign) the plus symbols, and then only when L(E,1) is non-zero.

Hence there is more to be done within eclib to get the signs right. I have set #22077 back to 'needs work' but it would also be possible for the sign to be checked on the Sage side.

comment:19 Changed 3 years ago by cremona

There's a new version of eclib (v20170104) over at #22077 and my last branch on this ticket (u/cremona/10256) works OK with it. I have not yet merged and tested u/wuthrich/ticket/10256 but I did test with wuthrich's independent test script.

comment:20 Changed 3 years ago by git

  • Commit changed from 724a20522ca193a859f85dfe275bb876613edccd to 49aa0395e6fb7f3d09b0724461db2dc4e7b69057

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

7e695e8updated eclib to v20170103
777fa61Merge branch 'u/cremona/22077' of git://trac.sagemath.org/sage into eclib
e101d21trac #10256: references
8fabe91updated eclib to v20170104
1c78871updated eclib to v20170104 again
b12bc13Merge branch 'u/cremona/22077' of git://trac.sagemath.org/sage into eclib
49aa039trac #10256: update latest version of eclib from #22077

comment:21 Changed 3 years ago by wuthrich

  • Status changed from needs_work to needs_review

With the latest changes to eclib, it looks good. Testing now.

comment:22 follow-up: Changed 3 years ago by wuthrich

  • Status changed from needs_review to needs_work

missed some doctests.

comment:23 in reply to: ↑ 22 Changed 3 years ago by cremona

Replying to wuthrich:

missed some doctests.

Dou mean that some are missing, or that they don't work?

comment:24 Changed 3 years ago by wuthrich

Sorry, I had to go, but now I am back. I meant: I missed updating the doctests in newforms.pyx with all the merging forth and back they got lost. Probably you are right and cleaning up could actually help. So one should take your new branch for #22077 and change the files in in elliptic_curves:

  • ell_modular_symbols
  • ell_rational_field
  • padic_lseries
  • padics

and in modular.pollack_stevens

  • space

There are also three misprints in newforms in libs.eclib which are better corrected in #22077.

Shall I do it? Or have you almost done it already?

comment:25 Changed 3 years ago by wuthrich

John, I fear we are doing parallel work. Upload yours.

I did the changes here, but I ran into a very strange compilation error for newforms.pyx. Cython did not recognise "pair". I added an import, but then I got other compilation errors like

_sp = self.nfs.plus_modular_symbol(_r, 0, int(base_at_infinity))
sage/libs/eclib/newforms.pyx:347:46: Call with wrong number of arguments (expected 1, got 3)

Honestly I don't understand. So if yours works, I am happy to ditch what I have done here.

comment:26 Changed 3 years ago by cremona

Sorry I totally messed up on the other ticket, and this one will have to wait until I sort that out, which will have to be Friday.

Correction (next morning): #22077 is fine and back to "needs review" -- see the remarks there. Nothing was messed up at all. I'll now finish cleaning up this one.

Last edited 3 years ago by cremona (previous) (diff)

comment:27 Changed 3 years ago by cremona

  • Branch changed from u/wuthrich/ticket/10256 to u/cremona/10256new
  • Commit 49aa0395e6fb7f3d09b0724461db2dc4e7b69057 deleted
  • Status changed from needs_work to needs_review

comment:28 Changed 3 years ago by cremona

OK so here's what I did. Starting from the new branch at #22077, I first used "git cherry-pick" to pick the 4 commits (6f517b3, 6301cc5, 6637227, e81588a) I had originally made here (which had been interspersed with eclib updates); only the first of those had some minor merge conflicts which I fixed. I then used rebase to sqush thos together into one commit ee879ee. Second, I did similarly with the 3 reviewer commits (dcacece, 724a205, e101d21), squashing into one (0db2a00). Lastly I added one more commit after testing revealed one doctest failing (49be7d8).

So I think that the 3 top commits on the new branch u/cremona/10256new contain all previous work while being based on u/cremona/22077new which is itself based on 7.5.rc1.

If there are additional changes needed here, that's fine as long as they are based on this (and don't require more changes to eclib itself...)

comment:29 Changed 3 years ago by wuthrich

Your branch doesn't exist, John. Mine still does not work.

comment:30 Changed 3 years ago by git

  • Commit set to 49be7d89ed221986f0d3f6df7debf401f1694a3a

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

cb3fd05#22077 update eclib to v20170104
ee879eework in progress on modular symbols
0db2a00trac #10256: reviewer patches: first part, highlight wrong values, references
49be7d810256: fix one doctest

comment:31 Changed 3 years ago by cremona

...it does help if you don't forget to actually push the branch to trac....it's there now

Last edited 3 years ago by cremona (previous) (diff)

comment:32 Changed 3 years ago by wuthrich

  • Status changed from needs_review to positive_review

All tests passed, the documentation, too. Thanks a lot.

comment:33 Changed 3 years ago by vbraun

  • Branch changed from u/cremona/10256new to 49be7d89ed221986f0d3f6df7debf401f1694a3a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.