Opened 12 years ago
Closed 6 years ago
#10256 closed enhancement (fixed)
make 1 modular symbols from eclib available to elliptic curves
Reported by:  wuthrich  Owned by:  John Cremona 

Priority:  major  Milestone:  sage7.5 
Component:  elliptic curves  Keywords:  modular symbols 
Cc:  John Cremona  Merged in:  
Authors:  John Cremona  Reviewers:  Chris Wuthrich 
Report Upstream:  N/A  Work issues:  
Branch:  49be7d8 (Commits, GitHub, GitLab)  Commit:  49be7d89ed221986f0d3f6df7debf401f1694a3a 
Dependencies:  #22077  Stopgaps: 
Description
This ticket is a follwup 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. padic Lseries.
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 12 years ago by
comment:2 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:3 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:4 Changed 8 years ago by
Milestone:  sage6.2 → sage6.3 

comment:5 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:6 Changed 6 years ago by
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 6 years ago by
Implementing this just went up my todo 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.
comment:8 Changed 6 years ago by
Excellent. I will help of course.
I am curious what Rubin and Mazur are up to, too.
comment:9 Changed 6 years ago by
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:11 Changed 6 years ago by
Authors:  → John Cremona 

Dependencies:  → #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 6 years ago by
Branch:  → u/cremona/10256 

Commit:  → 6637227842765bdf48022f7c7025d3bcb310b323 
Milestone:  sage6.4 → sage7.5 
Status:  new → 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 6 years ago by
Status:  needs_review → 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 6 years ago by
Branch:  u/cremona/10256 → u/wuthrich/ticket/10256 

Commit:  6637227842765bdf48022f7c7025d3bcb310b323 → 724a20522ca193a859f85dfe275bb876613edccd 
Reviewers:  → 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:
1b75fcc  Merge branch 'u/cremona/22077' of git://trac.sagemath.org/sage into eclib

4f51d79  Merge branch 'u/cremona/10256' of git://trac.sagemath.org/sage into eclib

dcacece  trac #10256: reviewer patch first part

724a205  trac #10256: highlight wrong values

comment:15 Changed 6 years ago by
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 6 years ago by
I am looking at this right now to see if it is really eclib and not the interface, Back soon...
comment:17 Changed 6 years ago by
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 6 years ago by
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 nonzero.
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 6 years ago by
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 6 years ago by
Commit:  724a20522ca193a859f85dfe275bb876613edccd → 49aa0395e6fb7f3d09b0724461db2dc4e7b69057 

Branch pushed to git repo; I updated commit sha1. New commits:
7e695e8  updated eclib to v20170103

777fa61  Merge branch 'u/cremona/22077' of git://trac.sagemath.org/sage into eclib

e101d21  trac #10256: references

8fabe91  updated eclib to v20170104

1c78871  updated eclib to v20170104 again

b12bc13  Merge branch 'u/cremona/22077' of git://trac.sagemath.org/sage into eclib

49aa039  trac #10256: update latest version of eclib from #22077

comment:21 Changed 6 years ago by
Status:  needs_work → needs_review 

With the latest changes to eclib, it looks good. Testing now.
comment:22 followup: 23 Changed 6 years ago by
Status:  needs_review → needs_work 

missed some doctests.
comment:23 Changed 6 years ago by
Replying to wuthrich:
missed some doctests.
Dou mean that some are missing, or that they don't work?
comment:24 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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.
comment:27 Changed 6 years ago by
Branch:  u/wuthrich/ticket/10256 → u/cremona/10256new 

Commit:  49aa0395e6fb7f3d09b0724461db2dc4e7b69057 
Status:  needs_work → needs_review 
comment:28 Changed 6 years ago by
OK so here's what I did. Starting from the new branch at #22077, I first used "git cherrypick" 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:30 Changed 6 years ago by
Commit:  → 49be7d89ed221986f0d3f6df7debf401f1694a3a 

comment:31 Changed 6 years ago by
...it does help if you don't forget to actually push the branch to trac....it's there now
comment:32 Changed 6 years ago by
Status:  needs_review → positive_review 

All tests passed, the documentation, too. Thanks a lot.
comment:33 Changed 6 years ago by
Branch:  u/cremona/10256new → 49be7d89ed221986f0d3f6df7debf401f1694a3a 

Resolution:  → fixed 
Status:  positive_review → closed 
The current eclib (eclib20100711.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.