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

Problem with old submodule #10453

Closed
loefflerd mannequin opened this issue Dec 9, 2010 · 14 comments
Closed

Problem with old submodule #10453

loefflerd mannequin opened this issue Dec 9, 2010 · 14 comments

Comments

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Dec 9, 2010

Weird things happen if you try to compute the old submodule of a Gamma1 modular forms space. E.g.

sage: CuspForms(Gamma1(11), 2).old_submodule() # crashes

This turns out to be because:

sage: ModularForms(Gamma1(11), 2).hecke_module_of_level(1).hecke_module_of_level(11)
Modular Forms space of dimension 2 for Congruence Subgroup Gamma0(11) of weight 2 over Rational Field

so the degeneracy level-raising map is getting computed for the wrong space!

Prerequisite for #11601. (Note that #11601 depends on a bunch of other stuff as well, but this ticket is independent of any of the other patches in the series.)


Apply attachment: trac_10453_gamma1_old_submodule.patch to the Sage library.

Component: modular forms

Author: David Loeffler

Reviewer: Johan Bosman

Merged: sage-4.7.2.alpha3

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

@loefflerd loefflerd mannequin added this to the sage-4.7.2 milestone Dec 9, 2010
@loefflerd loefflerd mannequin assigned craigcitro Dec 9, 2010
@loefflerd

This comment has been minimized.

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Dec 11, 2010

Attachment: trac_10453_gamma1_old_submodule.patch.gz

patch against 4.6.1.alpha3 + #8716

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Dec 11, 2010

comment:2

Here's a patch. It requires #8716 (but is independent of either #10450 or #10452). The fix was to make the various degeneracy matrix commands pass around an explicit target space, rather than just a target level, as the argument.

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Dec 11, 2010

Author: David Loeffler

@loefflerd loefflerd mannequin added the s: needs review label Dec 11, 2010
@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Dec 31, 2010

comment:3

Depends on #8716

@loefflerd

This comment has been minimized.

@loefflerd

This comment has been minimized.

@sagetrac-johanbosman
Copy link
Mannequin

sagetrac-johanbosman mannequin commented Aug 18, 2011

Reviewer: Johan Bosman

@sagetrac-johanbosman
Copy link
Mannequin

sagetrac-johanbosman mannequin commented Aug 18, 2011

comment:6

The code looks sound and is well documented. All long doctests pass. I played around with it and couldn't produce any false results. I did get the following while applying the patch:

applying trac_10453_gamma1_old_submodule.patch
patching file sage/modular/hecke/ambient_module.py
Hunk #10 succeeded at 883 with fuzz 1 (offset 6 lines).
now at: trac_10453_gamma1_old_submodule.patch

but according to this discussion, fuzz 1 is okay. Hence: positive review. ;).

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 9, 2011

comment:7

Replying to @sagetrac-johanbosman:

I did get the following while applying the patch:

applying trac_10453_gamma1_old_submodule.patch
patching file sage/modular/hecke/ambient_module.py
Hunk #10 succeeded at 883 with fuzz 1 (offset 6 lines).
now at: trac_10453_gamma1_old_submodule.patch

but according to this discussion, fuzz 1 is okay. Hence: positive review. ;).

In this case, you could re-export the patch and upload the rebased version.

IMHO one should inspect any fuzz anyway.

@nexttime

This comment has been minimized.

@loefflerd
Copy link
Mannequin Author

loefflerd mannequin commented Sep 11, 2011

comment:8

Replying to @nexttime:

IMHO one should inspect any fuzz anyway.

I have inspected the fuzz, which is caused by patch #10664 having gone in since this was written. It is not a problem; the issue fixed by #10664 is completely orthogonal to this one.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 11, 2011

comment:9

Replying to @loefflerd:

I have inspected the fuzz, which is caused by patch #10664 having gone in since this was written. It is not a problem; the issue fixed by #10664 is completely orthogonal to this one.

Thanks, I hadn't [yet].

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 17, 2011

Merged: sage-4.7.2.alpha3

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

1 participant