Opened 12 years ago

Closed 12 years ago

#2523 closed defect (fixed)

[with patch, with positive review] bug in modular symbols for GammaH subgroup

Reported by: was Owned by: craigcitro
Priority: major Milestone: sage-3.0.1
Component: modular forms Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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

Attachments (2)

trac-2523.patch (10.8 KB) - added by craigcitro 12 years ago.
trac-2523-pt2.patch (1.4 KB) - added by craigcitro 12 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 12 years ago by AlexGhitza

  • Component changed from number theory to modular forms
  • Owner changed from was to craigcitro

comment:2 Changed 12 years ago by AlexGhitza

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

comment:3 Changed 12 years ago by craigcitro

  • Status changed from new to assigned
  • Summary changed from bug in modular symbols for GammaH subgroup to [with patch, needs review] bug in modular symbols for GammaH subgroup

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.

Changed 12 years ago by craigcitro

comment:4 Changed 12 years ago by ncalexan

  • Summary changed from [with patch, needs review] bug in modular symbols for GammaH subgroup to [with patch, with positive review] bug in modular symbols for GammaH subgroup

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.

comment:5 Changed 12 years ago by AlexGhitza

Two comments:

  1. this patch is great; not only does it fix the current issue, but also fixes #2938. Awesome!
  1. 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.

Changed 12 years ago by craigcitro

comment:6 Changed 12 years ago by craigcitro

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

comment:7 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in Sage 3.0.1.alpha1

Note: See TracTickets for help on using tickets.