Opened 14 years ago
Closed 13 years ago
#2535 closed defect (fixed)
[with patch, positive review] Problem with cuspidal_subspace and new_subspace for modular symbols
Reported by: | craigcitro | Owned by: | craigcitro |
---|---|---|---|
Priority: | major | Milestone: | sage-3.3 |
Component: | modular forms | Keywords: | |
Cc: | AlexGhitza, cremona | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
There's some error with plus_submodule
and cuspidal_submodule
not being "commutative." Here's an example:
sage: M = ModularSymbols(11,2) sage: Mpc = M.plus_submodule().cuspidal_submodule() sage: Mcp = M.cuspidal_submodule().plus_submodule() sage: Mcp.q_expansion_basis(10) [ q - 2*q^2 - q^3 + 2*q^4 + q^5 + 2*q^6 - 2*q^7 - 2*q^9 + O(q^10) ] sage: Mpc.q_expansion_basis(10) --------------------------------------------------------------------------- <type 'exceptions.RuntimeError'> Traceback (most recent call last) /Users/craigcitro/<ipython console> in <module>() /sage/local/lib/python2.5/site-packages/sage/modular/modsym/space.py in q_expansion_basis(self, prec, algorithm) 458 algorithm = 'hecke' 459 if algorithm == 'hecke': --> 460 return Sequence(self._q_expansion_basis_hecke_dual(prec), cr=True) 461 elif algorithm == 'eigen': 462 return Sequence(self._q_expansion_basis_eigen(prec), cr=True) /sage/local/lib/python2.5/site-packages/sage/modular/modsym/space.py in _q_expansion_basis_hecke_dual(self, prec) 913 t = misc.verbose('computing basis to precision %s'%prec) 914 while V.dimension() < d and i >= 0: --> 915 v = [self.dual_hecke_matrix(n).column(i) for n in range(1,prec)] 916 t = misc.verbose('iteration: %s'%j,t) 917 X = M(v).transpose() /sage/local/lib/python2.5/site-packages/sage/modular/hecke/module.py in dual_hecke_matrix(self, n) 725 self._dual_hecke_matrices = {} 726 if not self._dual_hecke_matrices.has_key(n): --> 727 T = self._compute_dual_hecke_matrix(n) 728 self._dual_hecke_matrices[n] = T 729 return self._dual_hecke_matrices[n] /sage/local/lib/python2.5/site-packages/sage/modular/hecke/submodule.py in _compute_dual_hecke_matrix(self, n) 108 A = self.ambient_hecke_module().dual_hecke_matrix(n) 109 check = arith.gcd(self.level(), n) != 1 --> 110 return A.restrict(self.dual_free_module(), check=check) 111 112 def _compute_hecke_matrix(self, n): /sage/local/lib/python2.5/site-packages/sage/modular/hecke/submodule.py in dual_free_module(self, bound, anemic) 295 # failed 296 raise RuntimeError, "Computation of embedded dual vector space failed " + \ --> 297 "(cut down to rank %s, but should have cut down to rank %s)."%(V.rank(), self.rank()) 298 299 <type 'exceptions.RuntimeError'>: Computation of embedded dual vector space failed (cut down to rank 2, but should have cut down to rank 1).
I'll look at this soon.
Attachments (1)
Change History (12)
comment:1 Changed 14 years ago by
- Status changed from new to assigned
- Summary changed from Problem with cuspidal_subspace and new_subspace for modular symbols to [with patch, needs review] Problem with cuspidal_subspace and new_subspace for modular symbols
comment:2 Changed 14 years ago by
- Summary changed from [with patch, needs review] Problem with cuspidal_subspace and new_subspace for modular symbols to [with patch, with partial positive review] Problem with cuspidal_subspace and new_subspace for modular symbols
Patch looks good -- code is clean, doctests are good.
I cannot be the only judge on this patch because I am not expert in the functionality.
One change I would advocate is documenting the choices for use_sign
. See the source is not the best documentation :)
comment:3 Changed 14 years ago by
- Summary changed from [with patch, with partial positive review] Problem with cuspidal_subspace and new_subspace for modular symbols to [with patch, with partial positive and partial negative review] Problem with cuspidal_subspace and new_subspace for modular symbols
Negative review, since the code doesn't work on the example given below:
17:47 < wstein-2535> craigcitro -- what happens with this new code when the submodule we're trying to get the 17:47 < wstein-2535> dual of has both * eigenvalues? 17:48 < wstein-2535> I think this new code might just break then. 17:48 -!- roed_ [n=roed@c-98-216-48-4.hsd1.ma.comcast.net] has quit [] 17:48 < craigcitro> ah, you're saying i want an 'else' clause in the 'star' case. 17:48 < wstein-2535> At a minimum. 17:49 < wstein-2535> And also you *have* to use star in that case. 17:49 < wstein-2535> Or something. 17:49 < wstein-2535> You've only treated one cases, namely * = constant on subspace. 17:49 < wstein-2535> But this "use the *" trick should work more generaly for any *-equivariant module. 17:49 < craigcitro> oh, you're saying in that case, take the + and - submodules and then just take the sum 17:50 < craigcitro> (i.e. do the same thing i'd do in the case of one eigenvalue with each of them) 17:50 < wstein-2535> sage: M = ModularSymbols(43).cuspidal_submodule() 17:50 < wstein-2535> sage: S = M[0].plus_submodule() + M[1].minus_submodule() 17:50 < wstein-2535> sage: S.dual_free_module() 17:50 < wstein-2535> boom! 17:50 < wstein-2535> Yes, exactly.
comment:4 Changed 14 years ago by
- Cc AlexGhitza added
comment:5 Changed 14 years ago by
- Summary changed from [with patch, with partial positive and partial negative review] Problem with cuspidal_subspace and new_subspace for modular symbols to [with patch, needs work] Problem with cuspidal_subspace and new_subspace for modular symbols
Reclassify so this ticket is picked up properly by the various reports.
Cheers,
Michael
comment:6 Changed 14 years ago by
- Cc cremona added
comment:7 Changed 14 years ago by
comment:8 Changed 13 years ago by
- Summary changed from [with patch, needs work] Problem with cuspidal_subspace and new_subspace for modular symbols to [with patch, needs review] Problem with cuspidal_subspace and new_subspace for modular symbols
The attached patch addresses William's counterexample, and should be applied after Craig's patch.
Changed 13 years ago by
comment:9 Changed 13 years ago by
Final version of patch attached, with one or two small improvements over previous.
comment:10 Changed 13 years ago by
- Summary changed from [with patch, needs review] Problem with cuspidal_subspace and new_subspace for modular symbols to [with patch, positive review] Problem with cuspidal_subspace and new_subspace for modular symbols
comment:11 Changed 13 years ago by
- Milestone changed from sage-3.4.1 to sage-3.3
- Resolution set to fixed
- Status changed from assigned to closed
Merged in Sage 3.3.alpha2
Cheers,
Michael
The underlying bug was that we didn't use the star operator in addition to the Hecke operators when trying to determine the dual space to a space of modular symbols. I added this in, though there are several choices as to how to best do that. I added in at least two of these, and added a flag that chooses between them. I tested a handful of small examples, and picked the clear winner on these examples as a default. We should revisit this at some point and decide if that's really the best choice, or if there are tradeoffs with weight vs. level, etc.
I also added a few doctests here and there, since I was at it.