TestSuite failures for HeckeModule Homsets #12879

nthiery · 2012-04-25T00:25:30Z

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

The text was updated successfully, but these errors were encountered:

simon-king-jena · 2012-08-29T07:40:56Z

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 · 2012-08-29T07:43:08Z

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 · 2012-08-29T12:30:30Z

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 · 2012-08-29T12:34:19Z

comment:4

No, that didn't help.

nthiery · 2012-08-29T12:54:07Z

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 · 2017-07-27T08:56:05Z

Branch: u/chapoton/12879

fchapoton · 2017-07-27T08:56:05Z

Commit: 578b814

fchapoton · 2017-07-27T08:56:05Z

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 · 2017-07-27T08:56:05Z

Author: Frédéric Chapoton

fchapoton · 2017-07-27T17:04:17Z

comment:11

bot is morally green, please review

tscrim · 2017-07-28T17:00:31Z

comment:12

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

tscrim · 2017-07-28T17:00:31Z

Reviewer: Travis Scrimshaw

fchapoton · 2017-07-29T09:53:44Z

comment:19

done, please review.

New commits:

`dc917d4`	`trac 12879 implement a correct element`

koffie · 2017-07-29T09:58:18Z

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 · 2017-07-29T10:18:07Z

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 ?

sagetrac-git · 2017-07-29T11:22:19Z

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

`1ea2824`	`trac 12879 stronger check for scalar elements`

sagetrac-git · 2017-07-29T11:22:19Z

Changed commit from dc917d4 to 1ea2824

tscrim · 2017-07-30T17:05:07Z

comment:24

@vbraun Why the bump to 8.2?

koffie · 2017-08-13T21:52:57Z

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 · 2017-08-13T21:54:48Z

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 · 2017-08-14T01:14:36Z

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 · 2017-08-20T06:48:57Z

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 · 2017-08-20T12:55:35Z

comment:29

Is it possible to get more verbose output?

fchapoton · 2017-08-20T13:13:43Z

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 · 2017-08-20T16:40:53Z

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

sagetrac-git · 2017-08-20T17:49:49Z

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`

sagetrac-git · 2017-08-20T17:49:49Z

Changed commit from 1ea2824 to 761ac1d

fchapoton · 2017-08-20T17:50:15Z

comment:33

done

tscrim · 2017-08-20T18:10:09Z

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Maarten Derickx

tscrim · 2017-08-20T18:10:09Z

comment:34

Thank you.

vbraun · 2017-08-26T09:58:09Z

Changed branch from u/chapoton/12879 to 761ac1d

nthiery added this to the sage-5.11 milestone Apr 25, 2012

nthiery added c: modular forms labels Apr 25, 2012

nthiery assigned craigcitro Apr 25, 2012

jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014

fchapoton modified the milestones: sage-6.4, sage-8.1 Jul 27, 2017

fchapoton added the s: needs review label Jul 27, 2017

tscrim added s: positive review and removed s: needs review labels Jul 28, 2017

fchapoton added s: needs review and removed s: needs work labels Jul 29, 2017

vbraun modified the milestones: sage-8.1, sage-8.2 Jul 29, 2017

tscrim modified the milestones: sage-8.2, sage-8.1 Aug 20, 2017

tscrim added s: positive review and removed s: needs review labels Aug 20, 2017

vbraun removed the s: positive review label Aug 26, 2017

vbraun closed this as completed in ddf46df Aug 26, 2017

nthiery mentioned this issue Apr 27, 2012

Fix element and parent classes of Hom categories to be abstract, and simplify the Hom logic. #12876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TestSuite failures for HeckeModule Homsets #12879

TestSuite failures for HeckeModule Homsets #12879

nthiery commented Apr 25, 2012

simon-king-jena commented Aug 29, 2012

simon-king-jena commented Aug 29, 2012

simon-king-jena commented Aug 29, 2012

simon-king-jena commented Aug 29, 2012

nthiery commented Aug 29, 2012

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

tscrim commented Jul 28, 2017

tscrim commented Jul 28, 2017

fchapoton commented Jul 29, 2017

koffie commented Jul 29, 2017

fchapoton commented Jul 29, 2017

sagetrac-git mannequin commented Jul 29, 2017

sagetrac-git mannequin commented Jul 29, 2017

tscrim commented Jul 30, 2017

koffie commented Aug 13, 2017

koffie commented Aug 13, 2017

tscrim commented Aug 14, 2017

fchapoton commented Aug 20, 2017

koffie commented Aug 20, 2017

fchapoton commented Aug 20, 2017

tscrim commented Aug 20, 2017

sagetrac-git mannequin commented Aug 20, 2017

sagetrac-git mannequin commented Aug 20, 2017

fchapoton commented Aug 20, 2017

tscrim commented Aug 20, 2017

tscrim commented Aug 20, 2017

vbraun commented Aug 26, 2017

TestSuite failures for HeckeModule Homsets #12879

TestSuite failures for HeckeModule Homsets #12879

Comments

nthiery commented Apr 25, 2012

simon-king-jena commented Aug 29, 2012

simon-king-jena commented Aug 29, 2012

simon-king-jena commented Aug 29, 2012

simon-king-jena commented Aug 29, 2012

nthiery commented Aug 29, 2012

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

fchapoton commented Jul 27, 2017

tscrim commented Jul 28, 2017

tscrim commented Jul 28, 2017

fchapoton commented Jul 29, 2017

koffie commented Jul 29, 2017

fchapoton commented Jul 29, 2017

sagetrac-git mannequin commented Jul 29, 2017

sagetrac-git mannequin commented Jul 29, 2017

tscrim commented Jul 30, 2017

koffie commented Aug 13, 2017

koffie commented Aug 13, 2017

tscrim commented Aug 14, 2017

fchapoton commented Aug 20, 2017

koffie commented Aug 20, 2017

fchapoton commented Aug 20, 2017

tscrim commented Aug 20, 2017

sagetrac-git mannequin commented Aug 20, 2017

sagetrac-git mannequin commented Aug 20, 2017

fchapoton commented Aug 20, 2017

tscrim commented Aug 20, 2017

tscrim commented Aug 20, 2017

vbraun commented Aug 26, 2017