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

bug in modular symbols for GammaH subgroup #2523

Closed
williamstein opened this issue Mar 15, 2008 · 6 comments
Closed

bug in modular symbols for GammaH subgroup #2523

williamstein opened this issue Mar 15, 2008 · 6 comments

Comments

@williamstein
Copy link
Contributor

sage: ModularSymbols(GammaH(33,[1,2]),2).cuspidal_subspace()
Traceback (most recent call last):
...
KeyError: 11

Component: modular forms

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

@aghitza
Copy link

aghitza commented Apr 23, 2008

comment:2

I've done a bit of digging and gotten a bit closer to the source of the problem (which I think is actually in congroup.py):

sage: G = GammaH(6, [5])
sage: M = ModularSymbols(G, 2)
sage: B = M.boundary_space()
sage: bas = M.basis(); bas
((1,0), (3,2), (4,1))
sage: B(bas[0])
[Infinity] - [0]
sage: B(bas[2])
[0] - [-1/2]
sage: B(bas[1])
Traceback (most recent call last):
...
KeyError: 3

@craigcitro
Copy link
Member

comment:3

I'm attaching a fix. The fix also cleans up a few related functions. The problem was in the _coset_reduction_data functions -- specifically, one of the functions only generated data up to the square root of the level. However, in use, one needs more data -- at the very least, one needs to generate all data up to where the gcd of the term with N is at most square root of N. In that case, it seems easier to just generate all the data.

Added some doctests, though I'm not completely satisfied with my doctests: in particular, I couldn't come up with a doctest for _reduce_cusp that would have t == -1. I'd be happy if someone came up with one and added it.

I just noticed that this patch is actually in the tree where I'd already applied AlexGhitza's patch from #2995. Given that it's already been merged anyway, I'm just not going to worry about it.

@ncalexan
Copy link
Mannequin

ncalexan mannequin commented Apr 26, 2008

comment:4

Attachment: trac-2523.patch.gz

There are some typos, namely specfically in at least two places.

I'm worried about the lack of t==-1 doctest as well, but this looks good to me anyway.

@ncalexan ncalexan mannequin added the s: positive review label Apr 26, 2008
@aghitza
Copy link

aghitza commented Apr 26, 2008

comment:5

Two comments:

  1. this patch is great; not only does it fix the current issue, but also fixes [with documentation-only patch, positive reivew] ModularSymbols(GammaH(81, [10])).decomposition(); ModularSymbols(GammaH(8, [3])).decomposition() #2938. Awesome!

  2. regarding t = -1: looking at the code, I notice that t = -1 is returned only if, towards the end, u > N_over_2; but before that u goes through a bunch of reductions modulo d, so u is at most d. On the other hand, d=gcd(v,N), so the only way this can be bigger than half of N is if it's equal to N, i.e. if v = N. Having run a lot of random examples, I don't think t is ever -1, but it would be nice to have a theoretical proof of this (maybe William or John know this, or maybe they know how to construct an example with t=-1).

Having said this, I think we should be grateful for point (1) and merge, and figure out point (2) later.

@craigcitro
Copy link
Member

comment:6

Attachment: trac-2523-pt2.patch.gz

Added a small patch fixing the typos ncalexan pointed out. Also, I'm closing #2938 as fixed.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Apr 26, 2008

comment:7

Merged in Sage 3.0.1.alpha1

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Apr 26, 2008
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