TestSuite failures for HeckeModule Homsets

nthiery commented 12 years ago

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/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/categories/modules.py", line 221, in zero
    From: Free module generated by {1, 2, 3} over Integer Ring
  File "/opt/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/modular/hecke/homspace.py", line 111, in __call__
    return morphism.HeckeModuleMorphism_matrix(self, A, name)
  File "/opt/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/modular/hecke/morphism.py", line 116, in __init__
    MatrixMorphism.__init__(self, parent, A)
  File "/opt/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/modules/matrix_morphism.py", line 1191, in __init__
    parent.codomain().rank())(A)
  File "/opt/sage-5.0.beta11/local/lib/python2.7/site-packages/sage/matrix/matrix_space.py", line 543, in __call__
    return self.matrix(entries, copy=copy, coerce=coerce, rows=rows)
  File "/opt/sage-5.0.beta11/local/lib/python2.7/site-packages/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".

CC: @tscrim @koffie @JohnCremona @loefflerd @jonhanke

Component: modular forms

Author: Frédéric Chapoton

Branch/Commit: 761ac1d

Reviewer: Travis Scrimshaw, Maarten Derickx

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

simon-king-jena commented 12 years ago

comment:1

Nicolas suggested to me to move this from #12876 to here. I found the following steps needed:

Make HeckeModule_generic inherit from UniqueRepresentation. 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 sage-nt.
Provide the Hecke homsets with a __reduce__ method, so that (after the Hecke modules being unique) the homsets are identical.
Provide the Hecke homsets with an Element attribute, and let the constructors return instances of the element_class. Note that there is only one type of morphisms being returned, hence, having a default element class absolutely makes sense.
It was impossible to override the custom __call__ method of Hecke homsets by a proper _element_construtor_, since there is yet another custon __call__ method up in the class hierarchy.
Provide the Hecke homsets with a zero() method - the default one wouldn't work.
Provide the Hecke homsets with an _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), take an_element from the matrix space, and return the corresponding morphism.

With that approach, I get

        sage -t  -force_lib devel/sage/sage/modular/modsym/ambient.py # 1 doctests failed
        sage -t  -force_lib devel/sage/sage/modular/abvar/homspace.py # 1 doctests failed
        sage -t  -force_lib devel/sage/sage/modular/modform/constructor.py # 3 doctests failed
        sage -t  -force_lib devel/sage/sage/modular/quatalg/brandt.py # 3 doctests failed
        sage -t  -force_lib devel/sage/sage/modular/hecke/ambient_module.py # 2 doctests failed
        sage -t  -force_lib devel/sage/sage/modular/modsym/modsym.py # 2 doctests failed
        sage -t  -force_lib devel/sage/sage/modular/abvar/homology.py # 9 doctests failed
        sage -t  -force_lib devel/sage/sage/modular/modform/hecke_operator_on_qexp.py # 2 doctests failed

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.

simon-king-jena commented 12 years ago

comment:2

Replying to @simon-king-jena:

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.

simon-king-jena commented 12 years ago

comment:3

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.

simon-king-jena commented 12 years ago

comment:4

No, that didn't help.

nthiery commented 12 years ago

comment:5

Replying to @simon-king-jena:

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 on the short reviewer patch on #12876

Cheers, Nicolas

fchapoton commented 7 years ago

Branch: u/chapoton/12879

fchapoton commented 7 years ago

Commit: 578b814

fchapoton commented 7 years ago

comment:10

Here is a try. Not perfect, but better than right now.

New commits:

578b814 trac 12879 some care for Hecke modules homsets

fchapoton commented 7 years ago

Author: Frédéric Chapoton

fchapoton commented 7 years ago

comment:11

bot is morally green, please review

tscrim commented 7 years ago

comment:12

LGTM. At least there is nothing in the documentation that seems to put a restriction on what the matrices could be.

tscrim commented 7 years ago

Reviewer: Travis Scrimshaw

fchapoton commented 7 years ago

comment:13

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.

tscrim commented 7 years ago

comment:14

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?

fchapoton commented 7 years ago

comment:15

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.

koffie commented 7 years ago

comment:16

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.

fchapoton commented 7 years ago

comment:17

ok, thanks

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 578b814 to dc917d4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

dc917d4 trac 12879 implement a correct element

fchapoton commented 7 years ago

comment:19

done, please review.

New commits:

dc917d4 trac 12879 implement a correct element

koffie commented 7 years ago

comment:20

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

fchapoton commented 7 years ago

comment:21

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 ?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

1ea2824 trac 12879 stronger check for scalar elements

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from dc917d4 to 1ea2824

tscrim commented 7 years ago

comment:24

@vbraun Why the bump to 8.2?

koffie commented 7 years ago

comment:25

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.

koffie commented 7 years ago

comment:26

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?

tscrim commented 7 years ago

comment:27

@koffie 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.

@fchapoton I believe we are now claiming it passes the TestSuite, so I think we should explicitly add that as a doctest.

fchapoton commented 7 years ago

comment:28

no, the TestSuite still does not pass:

  The following tests failed: _test_category
Failure in _test_elements
The following tests failed: _test_elements

koffie commented 7 years ago

comment:29

Is it possible to get more verbose output?

fchapoton commented 7 years ago

comment:30

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/site-packages/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

tscrim commented 7 years ago

comment:31

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)?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

`e7132ee`	`Merge branch 'u/chapoton/12879' in 8.1.b2`
`761ac1d`	`trac 12879 adding TesTsuite`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 1ea2824 to 761ac1d

fchapoton commented 7 years ago

comment:33

done

tscrim commented 7 years ago

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Maarten Derickx

tscrim commented 7 years ago

comment:34

Thank you.

vbraun commented 7 years ago

Changed branch from u/chapoton/12879 to 761ac1d

sagemath / sage

TestSuite failures for HeckeModule Homsets #12879