Opened 8 years ago
Closed 3 years ago
#12879 closed defect (fixed)
TestSuite failures for HeckeModule Homsets
Reported by:  nthiery  Owned by:  craigcitro 

Priority:  major  Milestone:  sage8.1 
Component:  modular forms  Keywords:  
Cc:  tscrim, mderickx, cremona, davidloeffler, jonhanke  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw, Maarten Derickx 
Report Upstream:  N/A  Work issues:  
Branch:  761ac1d (Commits)  Commit:  761ac1d2b925153462e3529a374abedb8a8ccb07 
Dependencies:  Stopgaps: 
Description
Adding a Testsuite run in #12876 in the documentation of sage.categories.hecke_modules.HeckeModules?.ParentMethods?._Hom_ revealed the following bug:
sage: M = ModularForms(Gamma0(7), 4) sage: H = Hom(M); H sage: H.zero() Traceback (most recent call last): File "<ipython console>", line 1, in <module> File "cachefunc.pyx", line 1397, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (sage/misc/cachefunc.c:7026) File "/opt/sage5.0.beta11/local/lib/python2.7/sitepackages/sage/categories/modules.py", line 221, in zero From: Free module generated by {1, 2, 3} over Integer Ring File "/opt/sage5.0.beta11/local/lib/python2.7/sitepackages/sage/modular/hecke/homspace.py", line 111, in __call__ return morphism.HeckeModuleMorphism_matrix(self, A, name) File "/opt/sage5.0.beta11/local/lib/python2.7/sitepackages/sage/modular/hecke/morphism.py", line 116, in __init__ MatrixMorphism.__init__(self, parent, A) File "/opt/sage5.0.beta11/local/lib/python2.7/sitepackages/sage/modules/matrix_morphism.py", line 1191, in __init__ parent.codomain().rank())(A) File "/opt/sage5.0.beta11/local/lib/python2.7/sitepackages/sage/matrix/matrix_space.py", line 543, in __call__ return self.matrix(entries, copy=copy, coerce=coerce, rows=rows) File "/opt/sage5.0.beta11/local/lib/python2.7/sitepackages/sage/matrix/matrix_space.py", line 1372, in matrix return self.__matrix_class(self, entries=x, copy=copy, coerce=coerce) File "matrix_rational_dense.pyx", line 205, in sage.matrix.matrix_rational_dense.Matrix_rational_dense.__init__ (sage/matrix/matrix_rational_dense.c:5788) TypeError: entries must be coercible to a list or integer
TestSuite(H).run()
also fails for "_test_elements".
Change History (35)
comment:1 followup: ↓ 2 Changed 8 years ago by
comment:2 in reply to: ↑ 1 Changed 8 years ago by
Replying to SimonKing:
which isn't so bad, because most of them fail due to new class names from the category framework.
Sorry, that was wrong. More nasty errors happen as well.
comment:3 Changed 8 years ago by
Here is one particularly nasty thing. I stored some objects involved in errors. Reloading them, I find the following in a patched version (as suggested above):
sage: A = load("/home/king/SAGE/work/categories/modformA") sage: B = load("/home/king/SAGE/work/categories/modformB") sage: A Modular Symbols space of dimension 3 and level 5, weight 7, character [i], sign 1, over Number Field in c with defining polynomial x^2  402*i over its base field sage: B Modular Symbols space of dimension 3 and level 5, weight 7, character [i], sign 1, over Number Field in i with defining polynomial x^2 + 1 sage: A.boundary_space().dimension() 0 sage: K.<i> = QuadraticField(1) sage: x = polygen(K); L.<c> = K.extension(x^2  402*i) sage: chi = DirichletGroup(5, K).gen(0) sage: N = Newforms(chi, 7, base_ring = L) sage: Newforms(chi, 7, names='a') Traceback (most recent call last): ... TypeError: Unable to coerce x (=[1] [1] [1]) to a morphism in Set of Morphisms from Modular Symbols space of dimension 3 and level 5, weight 7, character [i], sign 1, over Number Field in i with defining polynomial x^2 + 1 to Boundary Modular Symbols space of level 5, weight 7, character [i] and dimension 1 over Number Field in c with defining polynomial x^2  402*i over its base field in Category of commutative additive groups sage: A.boundary_space().dimension() 1
Hence, A.boundary_space()
used to be of dimension zero, but during the computation, it is turned into dimension one!
That does not happen without the patch:
sage: A = load("/home/king/SAGE/work/categories/modformA") sage: B = load("/home/king/SAGE/work/categories/modformB") sage: A.boundary_space().dimension() 0 sage: K.<i> = QuadraticField(1) sage: x = polygen(K); L.<c> = K.extension(x^2  402*i) sage: chi = DirichletGroup(5, K).gen(0) sage: N = Newforms(chi, 7, base_ring = L) sage: Newforms(chi, 7, names='a') [q + a0*q^2 + (i*a0  5*i + 5)*q^3 + ((5*i  5)*a0 + 24*i)*q^4 + ((10*i  5)*a0  40*i  55)*q^5 + O(q^6)] sage: A.boundary_space().dimension() 0
Further digging reveals: A.boundary_space().dimension()
is indeed not constant! It relies on a list of known generators, that may grow. But with my patch, the homset assumes that the dimensions are constant. Bad me.
comment:4 followup: ↓ 5 Changed 8 years ago by
No, that didn't help.
comment:5 in reply to: ↑ 4 Changed 8 years ago by
Replying to SimonKing:
No, that didn't help.
Bah, unless you personnaly need this ticket to be fixed, you can just leave it to the number theory people. That's their problem :)
+1 one the short reviewer patch on #12876
Cheers,
Nicolas
comment:6 Changed 7 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:7 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:8 Changed 6 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:9 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:10 Changed 3 years ago by
 Branch set to u/chapoton/12879
 Commit set to 578b8144d44d00316b107eafd915c60ebecb604e
 Milestone changed from sage6.4 to sage8.1
 Status changed from new to needs_review
Here is a try. Not perfect, but better than right now.
New commits:
578b814  trac 12879 some care for Hecke modules homsets

comment:12 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM. At least there is nothing in the documentation that seems to put a restriction on what the matrices could be.
comment:13 Changed 3 years ago by
Oh, I did not think about that. Indeed, these Hecke modules are modules, so there are some operators that should act in a compatible way on both domain and codomain. Maybe we should not implement this "naive" version of an_element, which is probably not correct.
comment:14 Changed 3 years ago by
Well, there is no real documentation or checks in the code on this AFAICS. So as far as it is specified, any matrix will work. Perhaps also cc some number theory people to get their opinion?
comment:15 Changed 3 years ago by
 Cc mderickx cremona davidloeffler jonhanke added
To specialists of modular forms and symbols, do you agree with the proposed implementation of "an_element" for the space of Hecke module morphisms ? In short, we just take a random matrix, and declare that it is a morphism, which seems dubious.
comment:16 Changed 3 years ago by
No, since it does not take into account that Hecke module are modules over some Hecke algebra. So indeed as you say earlier, morphisms should commute with the action of the Hecke operators on both sides. I think creating a valid nonzero element (if it exists, which is very often not the case) is something that is not trivial to implement. So I would maybe always just return the zero morphism. If domain and codomain are equal you could do something like _.hecke_operator(2)
if you want a more typical element.
comment:18 Changed 3 years ago by
 Commit changed from 578b8144d44d00316b107eafd915c60ebecb604e to dc917d4e81a9f7daf8560748c0747371440d75d8
Branch pushed to git repo; I updated commit sha1. New commits:
dc917d4  trac 12879 implement a correct element

comment:19 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:20 Changed 3 years ago by
I also looked at the code in __call__
, and the part that handles the case that A is an element of the basing only makes sense if the domain and codomain are actually the same space! If A is in the base ring but the domain and the codomain are unequal we should throw an Error.
New commits:
dc917d4  trac 12879 implement a correct element

comment:21 Changed 3 years ago by
The call of identity matrix will raise an error if the dimensions are not the same. Do you want an explicit check that domain == codomain ?
comment:22 Changed 3 years ago by
 Commit changed from dc917d4e81a9f7daf8560748c0747371440d75d8 to 1ea2824e6813a8f2bcaaae332537681126aded03
Branch pushed to git repo; I updated commit sha1. New commits:
1ea2824  trac 12879 stronger check for scalar elements

comment:23 Changed 3 years ago by
 Milestone changed from sage8.1 to sage8.2
comment:24 Changed 3 years ago by
@vbraun Why the bump to 8.2?
comment:25 Changed 3 years ago by
Hi Sorry for not reacting earlier. I see that you added the explicit check for domain == codomain
which is good! It is possible to construct hecke modules which have noting to do with each other that still have the same rank. And then asking for the identity matrix makes no sense, since it is a linear map that sends whichever basis we computed for the first space to a basis we computed for the second space, and since the basis are quite random so is the map. In particular it will rarely be a hecke module morphism.
comment:26 Changed 3 years ago by
I've looked at the code and I can give a positive review on the mathematical content of this ticket. I have no Idea what all the
TestSuite
stuff is about. So is there someone else who can say that that part makes sense?
comment:27 Changed 3 years ago by
@mderickx TestSuite
runs a bunch of "standard" tests on the object (i.e., all of the _test_*
methods of an object) as a consistency check. Also, if you add your (real) name to the reviewers.
@chapoton I believe we are now claiming it passes the TestSuite
, so I think we should explicitly add that as a doctest.
comment:28 Changed 3 years ago by
no, the TestSuite? still does not pass:
The following tests failed: _test_category Failure in _test_elements The following tests failed: _test_elements
comment:29 Changed 3 years ago by
Is it possible to get more verbose output?
comment:30 Changed 3 years ago by
oh, well, this says
Running the test suite of self.an_element() running ._test_category() . . . fail Traceback (most recent call last): File "/home/chapoton/sage/local/lib/python2.7/sitepackages/sage/misc/sage_unittest.py", line 293, in run test_method(tester = tester) File "sage/structure/element.pyx", line 687, in sage.structure.element.Element._test_category (/home/chapoton/sage/src/build/cythonized/sage/structure/element.c:6380) tester.assert_(isinstance(self, self.parent().category().element_class)) File "/home/chapoton/sage/local/lib/python2.7/unittest/case.py", line 422, in assertTrue raise self.failureException(msg) AssertionError: False is not true
comment:31 Changed 3 years ago by
That is a common issue for Homsets because of it creating elements that are not specifically an instance of its unique Element
class. Currently, we don't have the infastructure to really work around this.
Could we add a TestSuite
that skips the _test_elements
(this is done for other Homsets too)?
comment:32 Changed 3 years ago by
 Commit changed from 1ea2824e6813a8f2bcaaae332537681126aded03 to 761ac1d2b925153462e3529a374abedb8a8ccb07
comment:33 Changed 3 years ago by
done
comment:34 Changed 3 years ago by
 Milestone changed from sage8.2 to sage8.1
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Maarten Derickx
 Status changed from needs_review to positive_review
Thank you.
comment:35 Changed 3 years ago by
 Branch changed from u/chapoton/12879 to 761ac1d2b925153462e3529a374abedb8a8ccb07
 Resolution set to fixed
 Status changed from positive_review to closed
Nicolas suggested to me to move this from #12876 to here. I found the following steps needed:
HeckeModule_generic
inherit fromUniqueRepresentation
. I guess it was for a reason that William did not always want modular forms being cached (there is a cache, but it isn't always used). Since nowadays the cache has weak keys, it shouldn't be so bad. I'll ask on sagent.__reduce__
method, so that (after the Hecke modules being unique) the homsets are identical.Element
attribute, and let the constructors return instances of theelement_class
. Note that there is only one type of morphisms being returned, hence, having a default element class absolutely makes sense.__call__
method of Hecke homsets by a proper_element_construtor_
, since there is yet another custon__call__
method up in the class hierarchy.zero()
method  the default one wouldn't work._an_element_
method. The idea is: Domain and codomain have a known dimension, and the morphisms are given by matrices anyway. Hence, I construct the corresponding matrix space (which I made a lazy attribute to the Hecke homsets), takean_element
from the matrix space, and return the corresponding morphism.With that approach, I get
which isn't so bad, because most of them fail due to new class names from the category framework.
Suggestion: I provide a small reviewer patch at #12876 making the
TestSuite
skip more tests, and then a proper solution will be dealt with here.