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

Membership testing for modular forms subspaces is hopeless #5995

Closed
loefflerd mannequin opened this issue May 6, 2009 · 7 comments
Closed

Membership testing for modular forms subspaces is hopeless #5995

loefflerd mannequin opened this issue May 6, 2009 · 7 comments

Comments

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented May 6, 2009

This is pretty poor, IMHO:

sage: M = ModularForms(17, 4)
sage: S = M.cuspidal_submodule()
sage: M.0 == S.0
True
sage: M.0 in S
False

As far as I can tell at a glance this is happening because S.__call__(x) tests whether or not the parent of x has a canonical inclusion map to S; it should probably be testing whether the parent of x has a canonical inclusion map to the ambient space of S.

Once the above is fixed we should also have a method is_cuspidal() for modular forms objects, which would be secretly just self in self.parent().cuspidal_submodule(). A corresponding is_eisenstein() would be good, too.

CC: @craigcitro

Component: modular forms

Author: David Loeffler

Reviewer: John Cremona

Merged: 4.0.1.alpha0

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

@loefflerd loefflerd mannequin added this to the sage-4.0.1 milestone May 6, 2009
@loefflerd loefflerd mannequin assigned craigcitro May 6, 2009
@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 12, 2009

apply after #4357 and #5736

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented May 12, 2009

comment:2

Attachment: trac_5995.patch.gz

Here's a patch, which adds is_cuspidal, is_eisenstein, is_new and is_old, and corrects a funny glitch whereby elliptic curve newforms consistently claimed not to be cuspidal :-) I wrote the patch and ran tests with this and everything else (including the not-yet-fully-refereed #5968) installed simultaneously, but it should at least apply as long as you have the patches at #4357 and #5736 installed.

@loefflerd loefflerd mannequin added the s: needs review label May 12, 2009
@JohnCremona
Copy link
Member

comment:4

Looks good to me. Patch applies fine to 4.0 and tests in sage/modular/{modform,hecke} pass.

@mwhansen
Copy link
Contributor

mwhansen commented Jun 1, 2009

comment:5

Merged in 4.0.1.alpha0.

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2009

Reviewer: John Cremona

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2009

Author: David Loeffler

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2009

Merged: 4.0.1.alpha0

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