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)
Change History (9)
comment:1 Changed 12 years ago by
- Component changed from number theory to modular forms
- Owner changed from was to craigcitro
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
- 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
comment:4 Changed 12 years ago by
- 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
Two comments:
- this patch is great; not only does it fix the current issue, but also fixes #2938. Awesome!
- 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
comment:6 Changed 12 years ago by
Added a small patch fixing the typos ncalexan pointed out. Also, I'm closing #2938 as fixed.
comment:7 Changed 12 years ago by
- Resolution set to fixed
- Status changed from assigned to closed
Merged in Sage 3.0.1.alpha1
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):