Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major extensions to local components code #26635

Closed
loefflerd mannequin opened this issue Nov 4, 2018 · 37 comments
Closed

Major extensions to local components code #26635

loefflerd mannequin opened this issue Nov 4, 2018 · 37 comments

Comments

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Nov 4, 2018

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 2-adic 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 2-adic 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
]

Component: modular forms

Keywords: local-components

Author: David Loeffler

Branch/Commit: c165835

Reviewer: David Roe

Issue created by migration from https://trac.sagemath.org/ticket/26635

@loefflerd loefflerd mannequin added this to the sage-8.5 milestone Nov 4, 2018
@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Nov 4, 2018

Branch: u/davidloeffler/minimal_twist_new

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Nov 4, 2018

Commit: 37ce61f

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Nov 4, 2018

New commits:

1b55638Add a minimal twist function for newform objects
37ce61fLocal components: handle many previously-unimplemented cases

@loefflerd loefflerd mannequin added the s: needs review label Nov 4, 2018
@loefflerd

This comment has been minimized.

@loefflerd

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 4, 2018

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

482e00aFix non-ascii chars, non-Py3 code etc picked up by patchbot

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 4, 2018

Changed commit from 37ce61f to 482e00a

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Nov 4, 2018

comment:6

I just committed a fix for some minor coding-style violations picked up by the patchbot.

@roed314
Copy link
Contributor

roed314 commented Apr 28, 2021

Changed branch from u/davidloeffler/minimal_twist_new to u/roed/minimal_twist_new

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 28, 2021

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

f8160a3Fix trivial test failures

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 28, 2021

Changed commit from 482e00a to f8160a3

@roed314
Copy link
Contributor

roed314 commented Apr 28, 2021

comment:9

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
    ]

@roed314
Copy link
Contributor

roed314 commented Apr 28, 2021

comment:10

Nope, merge still failed. I'll try a more recent version of Sage soon.

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Apr 29, 2021

comment:11

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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2021

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

0cef4b0Merge branch 'u/roed/minimal_twist_new' of git://trac.sagemath.org/sage into t/26635/minimal_twist_new

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2021

Changed commit from f8160a3 to 0cef4b0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2021

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

1681b79Fix doctest failure

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 30, 2021

Changed commit from 0cef4b0 to 1681b79

@roed314
Copy link
Contributor

roed314 commented Apr 30, 2021

comment:14

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!

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 1, 2021

comment:15

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?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 5, 2021

Changed commit from 1681b79 to c2267ce

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 5, 2021

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

c2267ceMostly style changes, one potential speedup

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 5, 2021

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

d0c9d58Mark fragile doctest as random

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 5, 2021

Changed commit from c2267ce to d0c9d58

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 5, 2021

Changed commit from d0c9d58 to 7d4dd1b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 5, 2021

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

7d4dd1bSome more style fixes

@roed314
Copy link
Contributor

roed314 commented May 5, 2021

comment:19

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 2-adic cases?

I'm sorry this took so long to get reviewed!

@roed314
Copy link
Contributor

roed314 commented May 5, 2021

Reviewer: David Roe

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 5, 2021

comment:20

Thanks for the review!

Your ticket for p-adic unit groups looks interesting & obviously it would be nice to unify it with the local-component code; but I doubt it would lead to much of a speed increase. In local-component computations, the computation time is overwhelmingly dominated by computing modular-symbol spaces and Hecke operators on them. The smoothchar.py stuff is just there to give a user-friendly way of interacting with the results of the modsym computation, it is computationally trivial in itself.

@fchapoton fchapoton modified the milestones: sage-8.5, sage-9.4 May 10, 2021
@vbraun
Copy link
Member

vbraun commented May 11, 2021

comment:22

PDF docs don't build

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 13, 2021

comment:23

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?

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 13, 2021

comment:24

Wait, I found the error: exactly two characters missing in a single docstring in smoothchar.py. Testing, will upload shortly.

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 13, 2021

Changed branch from u/roed/minimal_twist_new to u/davidloeffler/minimal_twist_new

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 13, 2021

Changed commit from 7d4dd1b to c165835

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 13, 2021

comment:26

Right, that should fix it. Given the microscopic size of the change, I presume it is valid to reinstate the positive review.


New commits:

c165835Fix broken latex in docstring

@roed314
Copy link
Contributor

roed314 commented May 13, 2021

comment:27

Yep, I agree with the positive review.

@vbraun
Copy link
Member

vbraun commented Jun 6, 2021

Changed branch from u/davidloeffler/minimal_twist_new to c165835

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants