Opened 4 years ago
Closed 12 months ago
#26635 closed enhancement (fixed)
Major extensions to local components code
Reported by:  davidloeffler  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  modular forms  Keywords:  localcomponents 
Cc:  Merged in:  
Authors:  David Loeffler  Reviewers:  David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  c165835 (Commits, GitHub, GitLab)  Commit:  c165835c65b91c1badecd3aae58fa1b031f74a2b 
Dependencies:  Stopgaps: 
Description (last modified by )
Some while ago, I contributed to Sage some code for computing the local component (at a given prime) of the automorphic representation attached to a modular form, based on algorithms from a paper I wrote with Jared Weinstein.
The existing implementation is incomplete in several ways:
 it throws an error if the given modular form doesn't have minimal level at p among its character twists;
 if the local component is supercuspidal, induced from a character of a ramified quadratic extension, then it doesn't completely compute the character, it just returns a string saying which extension it comes from;
 in supercuspidal 2adic cases where the level is divisible by 64, it fails because a certain quotient group that comes up in the computation isn't cyclic.
I have now reworked the code to fill in all of the above gaps. It should now work for all newforms and all primes, with the exception of certain nasty 2adic cases where no nice parametrisation exists. In particular, with the new code the following example works, fulfilling a request from Ariel Pacetti:
sage: E = EllipticCurve("575c1") sage: Pi = E.newform().local_component(5) sage: Pi.characters() [ Character of Q_5*, of level 1, mapping 2 > 1/2*a, 5 > 1/2*a + 2, Character of Q_5*, of level 1, mapping 2 > 1/2*a, 5 > 1/2*a + 2 ]
Change History (28)
comment:1 Changed 4 years ago by
 Branch set to u/davidloeffler/minimal_twist_new
comment:2 Changed 4 years ago by
 Commit set to 37ce61fee64a96db57abeaf5a41ac24ed3c00e98
 Status changed from new to needs_review
comment:3 Changed 4 years ago by
 Description modified (diff)
comment:4 Changed 4 years ago by
 Description modified (diff)
comment:5 Changed 4 years ago by
 Commit changed from 37ce61fee64a96db57abeaf5a41ac24ed3c00e98 to 482e00a6c12d5d87fca8ac23aa1daf5edea3f9b9
Branch pushed to git repo; I updated commit sha1. New commits:
482e00a  Fix nonascii chars, nonPy3 code etc picked up by patchbot

comment:6 Changed 4 years ago by
I just committed a fix for some minor codingstyle violations picked up by the patchbot.
comment:7 Changed 13 months ago by
 Branch changed from u/davidloeffler/minimal_twist_new to u/roed/minimal_twist_new
comment:8 Changed 13 months ago by
 Commit changed from 482e00a6c12d5d87fca8ac23aa1daf5edea3f9b9 to f8160a3c07931345e8c1a8ee33740d75f9c319d8
Branch pushed to git repo; I updated commit sha1. New commits:
f8160a3  Fix trivial test failures

comment:9 Changed 13 months ago by
I hopefully fixed the merge failure and a couple of deprecation warnings. There seems to be one actual doctest failure:
File "src/sage/modular/local_comp/local_comp.py", line 704, in sage.modular.local_comp.local_comp.PrimitiveSupercuspidal.characters Failed example: Newforms(DirichletGroup(64, QQ).1, 2, names='a')[0].local_component(2).characters() # long time Expected: [ Character of unramified extension Q_2(s)* (s^2 + s + 1 = 0), of level 3, mapping s > 1, 2*s + 1 > 1/4*a0, 4*s + 1 > 1, 1 > 1, 2 > 1, Character of unramified extension Q_2(s)* (s^2 + s + 1 = 0), of level 3, mapping s > 1, 2*s + 1 > 1/4*a0, 4*s + 1 > 1, 1 > 1, 2 > 1 ] Got: [ Character of unramified extension Q_2(s)* (s^2 + s + 1 = 0), of level 3, mapping s > 1, 2*s + 1 > 1/2*a0, 4*s + 1 > 1, 1 > 1, 2 > 1, Character of unramified extension Q_2(s)* (s^2 + s + 1 = 0), of level 3, mapping s > 1, 2*s + 1 > 1/2*a0, 4*s + 1 > 1, 1 > 1, 2 > 1 ]
comment:10 Changed 13 months ago by
Nope, merge still failed. I'll try a more recent version of Sage soon.
comment:11 Changed 13 months ago by
Nice to see some movement on this at long last, thanks for taking a look at it! The doctest failure looks harmless to me, my guess is that Sage has changed its mind about which generator to use for the coeff field of the newform. Let me know if you have trouble getting the merge to work, I can have a go myself (if I still remember how).
comment:12 Changed 13 months ago by
 Commit changed from f8160a3c07931345e8c1a8ee33740d75f9c319d8 to 0cef4b09472bc5f69e21eed99ec46f9b36a913e3
Branch pushed to git repo; I updated commit sha1. New commits:
0cef4b0  Merge branch 'u/roed/minimal_twist_new' of git://trac.sagemath.org/sage into t/26635/minimal_twist_new

comment:13 Changed 13 months ago by
 Commit changed from 0cef4b09472bc5f69e21eed99ec46f9b36a913e3 to 1681b79a2a889c9a9d21eed4d4eea406ed06196c
Branch pushed to git repo; I updated commit sha1. New commits:
1681b79  Fix doctest failure

comment:14 Changed 13 months ago by
I got the merge to work (and have adjusted the doctest to what's currently returned). I'll try to read through the code soon, and let you know if I have questions!
comment:15 Changed 13 months ago by
I checked about the failing long doctest. It is indeed unrelated to the code at hand, and arises because sage is using a random algorithm to choose a generator of the coeff field of the newform. Shall we just flag it as # random?
comment:16 Changed 13 months ago by
 Commit changed from 1681b79a2a889c9a9d21eed4d4eea406ed06196c to c2267ce939fdf1ffdfe0908025e8b3253487fe74
Branch pushed to git repo; I updated commit sha1. New commits:
c2267ce  Mostly style changes, one potential speedup

comment:17 Changed 13 months ago by
 Commit changed from c2267ce939fdf1ffdfe0908025e8b3253487fe74 to d0c9d58f3323d86c5344b57fcd74f469ecef78d9
Branch pushed to git repo; I updated commit sha1. New commits:
d0c9d58  Mark fragile doctest as random

comment:18 Changed 13 months ago by
 Commit changed from d0c9d58f3323d86c5344b57fcd74f469ecef78d9 to 7d4dd1bc2c9899b2c1261676f858e1eae9916d7a
Branch pushed to git repo; I updated commit sha1. New commits:
7d4dd1b  Some more style fixes

comment:19 Changed 13 months ago by
 Reviewers set to David Roe
 Status changed from needs_review to positive_review
I read through the code and it looks good. I made various minor changes to better conform to Sage's style guide. The one substantive change I made was to move the line t = self.discrete_log(n, p)
outside a loop.
I'm going to give this a positive review now, but we should keep an eye on the patchbot results. I'm also hopeful that I can help with improving speed eventually using #26849 (which still needs work). Finally, what's the theoretical status on the missing 2adic cases?
I'm sorry this took so long to get reviewed!
comment:20 Changed 13 months ago by
Thanks for the review!
Your ticket for padic unit groups looks interesting & obviously it would be nice to unify it with the localcomponent code; but I doubt it would lead to much of a speed increase. In localcomponent computations, the computation time is overwhelmingly dominated by computing modularsymbol spaces and Hecke operators on them. The smoothchar.py stuff is just there to give a userfriendly way of interacting with the results of the modsym computation, it is computationally trivial in itself.
comment:21 Changed 13 months ago by
 Milestone changed from sage8.5 to sage9.4
comment:22 followup: ↓ 23 Changed 13 months ago by
 Status changed from positive_review to needs_work
PDF docs don't build
comment:23 in reply to: ↑ 22 Changed 13 months ago by
Replying to vbraun:
PDF docs don't build
Volker: I cannot reproduce this failure on my machine (since pdf doc building doesn't work on my system for unrelated reasons). Can you post the relevant log file?
comment:24 Changed 13 months ago by
Wait, I found the error: exactly two characters missing in a single docstring in smoothchar.py
. Testing, will upload shortly.
comment:25 Changed 13 months ago by
 Branch changed from u/roed/minimal_twist_new to u/davidloeffler/minimal_twist_new
comment:26 Changed 13 months ago by
 Commit changed from 7d4dd1bc2c9899b2c1261676f858e1eae9916d7a to c165835c65b91c1badecd3aae58fa1b031f74a2b
 Status changed from needs_work to positive_review
Right, that should fix it. Given the microscopic size of the change, I presume it is valid to reinstate the positive review.
New commits:
c165835  Fix broken latex in docstring

comment:27 Changed 13 months ago by
Yep, I agree with the positive review.
comment:28 Changed 12 months ago by
 Branch changed from u/davidloeffler/minimal_twist_new to c165835c65b91c1badecd3aae58fa1b031f74a2b
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Add a minimal twist function for newform objects
Local components: handle many previouslyunimplemented cases