#4357: [with patch, with positive review after minor changes] modular forms --
new
subspace used to work and now broken
---------------------------+------------------------------------------------
Reporter: was | Owner: davidloeffler
Type: enhancement | Status: assigned
Priority: major | Milestone: sage-4.0
Component: modular forms | Keywords:
---------------------------+------------------------------------------------
Comment(by craigcitro):
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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/4357#comment:5>
Sage <http://sagemath.org/>
Sage - Open Source Mathematical Software: Building the Car Instead of
Reinventing the Wheel
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en
-~----------~----~----~----~------~----~------~--~---
