Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#4357 closed enhancement (fixed)

[with patch, with positive review] modular forms -- new subspace used to work and now broken

Reported by: was Owned by: davidloeffler
Priority: major Milestone: sage-4.0
Component: modular forms Keywords:
Cc: Merged in: 4.0.alpha0
Authors: David Loeffler Reviewers: Craig Citro
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

This used to work and is now broken. It is used in my modular forms book.

sage: CuspForms(45).new_subspace()
Traceback (most recent call last):
...
NotImplementedError: computation of new submodule not yet implemented

Attachments (2)

trac_4357.patch (29.3 KB) - added by davidloeffler 11 years ago.
patch against 3.4.2.alpha0
trac_4357_pt2.patch (4.6 KB) - added by davidloeffler 11 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by zimmerma

  • Summary changed from modualr forms -- new subspace used to work and now broken to modular forms -- new subspace used to work and now broken

comment:2 Changed 11 years ago by craigcitro

  • Status changed from new to assigned

This functionality was removed because it was broken. Indeed, the result returned in William's book is incorrect -- it should be:

CuspForms(Gamma0(45), 2, prec=14).new_subspace().basis()
(q + q^2 - q^4 - q^5 - 3*q^8 - q^10 + 4*q^11 - 2*q^13 + O(q^14),)

William and I have discussed a fix for this, which I'll take care of soon, unless William beats me to it.

Changed 11 years ago by davidloeffler

patch against 3.4.2.alpha0

comment:3 Changed 11 years ago by davidloeffler

  • Owner changed from craigcitro to davidloeffler
  • Status changed from assigned to new
  • Summary changed from modular forms -- new subspace used to work and now broken to [with patch, needs review] modular forms -- new subspace used to work and now broken

I have uploaded a patch which I think fixes this -- it certainly gives the above result for level 45 and weight 2.

comment:4 Changed 11 years ago by davidloeffler

  • Status changed from new to assigned

comment:5 Changed 11 years ago by craigcitro

  • Summary changed from [with patch, needs review] modular forms -- new subspace used to work and now broken to [with patch, with positive review after minor changes] modular forms -- new subspace used to work and now broken

REFEREE REPORT:

Looks good! I'm happy to see this code get merged, modulo a few really tiny things:

  • On line 238 of modular/modform/ambient.py, I think a line needs to be split.
  • In the same file, at line 521, I think we should reverse the order of the cuspidal and Eisenstein parts, so that it matches the order we have for the basis of the modular forms space itself. (Of course, there's no real reason it should be one order over another, but it should be consistent, and the basis code has been around much longer.)
  • Note that there's an issue with the atkin_lehner_eigenvalue code, but this is fixed in your patch at #5262, so I'm not worried. (I'm about to review that one.)
  • On this last one, I mostly have a question -- I could just be confused. You've added a new argument t to all the _degeneracy_raising_matrix functions, which makes sense with the other changes you've made. But why do you raise a RuntimeError if the value isn't 1? Shouldn't it be something like a NotImplementedError, or am I missing something? Or if no argument but t=1 makes sense (for a reason I don't see), then I think it should be a ValueError.

Once those get fixed, I'm happy to see this code go in.

comment:6 Changed 11 years ago by davidloeffler

As for the t argument thing: for modular symbols, where the raising matrices are complicated trace-like operators, the argument t is not used, since the degeneracy raising matrix for all values of t is calculated from the one for t = 1 by composing with a Hecke operator. But for modular forms, the degeneracy raising matrices are pretty easy (since we're working in a "cohomological" setup rather than a "homological" one) so it is easier to calculate them directly for each t.

Hence if _compute_degeneracy_raising matrix is getting called on a modular symbols space with t != 1, that's a runtime error, but on a modular forms space that's not the case. That should be explained in docstrings somewhere; I'll do a follow-up patch.

David

comment:7 Changed 11 years ago by davidloeffler

OK, here's a new patch. This makes absolutely no changes in functionality, but it moves stuff around a bit to clarify what's going on with this mysterious "t" argument -- the calculation of the other degeneracy maps from the t=1 one now happens entirely in modsym/ambient.py, which is only right and proper.

I've swapped the order of the sum of cuspidal and Eisenstein parts, but this is solely cosmetic: addition of subspaces of vector spaces in Sage *is* commutative (because the subspace constructor echelon-reduces the basis vectors), and one can check that even with the old patch, for ModularForms(9, 12).new_subspace(), the single new Eisenstein series gets listed last, not first.

Changed 11 years ago by davidloeffler

comment:8 Changed 11 years ago by craigcitro

  • Summary changed from [with patch, with positive review after minor changes] modular forms -- new subspace used to work and now broken to [with patch, with positive review] modular forms -- new subspace used to work and now broken

Excellent ... I'm very happy with all the changes. The t stuff is cleaner, and good point about the swapping being cosmetic -- I was reading it and thought "oh, that should match the order for the ambient space" without actually realizing that it was irrelevant, since the subspace will have an echelon basis.

Two thumbs way up.

comment:9 Changed 11 years ago by mabshoff

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

Merged both patches in Sage 4.0.alpha0.

Cheers,

Michael

comment:10 Changed 11 years ago by davidloeffler

  • Authors set to David Loeffler
  • Merged in set to 4.0.alpha0
  • Reviewers set to Craig Citro
Note: See TracTickets for help on using tickets.