Opened 8 years ago

Closed 2 years ago

#12879 closed defect (fixed)

TestSuite failures for HeckeModule Homsets

Reported by: nthiery Owned by: craigcitro
Priority: major Milestone: sage-8.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/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".

Change History (35)

comment:1 follow-up: Changed 7 years ago by SimonKing

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.

comment:2 in reply to: ↑ 1 Changed 7 years ago by SimonKing

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 7 years ago by SimonKing

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 follow-up: Changed 7 years ago by SimonKing

No, that didn't help.

comment:5 in reply to: ↑ 4 Changed 7 years ago by nthiery

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

Cheers,

Nicolas

Last edited 5 years ago by nthiery (previous) (diff)

comment:6 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:7 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:9 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:10 Changed 2 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/12879
  • Commit set to 578b8144d44d00316b107eafd915c60ebecb604e
  • Milestone changed from sage-6.4 to sage-8.1
  • Status changed from new to needs_review

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


New commits:

578b814trac 12879 some care for Hecke modules homsets

comment:11 Changed 2 years ago by chapoton

  • Cc tscrim added

bot is morally green, please review

comment:12 Changed 2 years ago by tscrim

  • 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 2 years ago by chapoton

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 2 years ago by tscrim

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 2 years ago by chapoton

  • 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 2 years ago by mderickx

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:17 Changed 2 years ago by chapoton

  • Status changed from positive_review to needs_work

ok, thanks

comment:18 Changed 2 years ago by git

  • Commit changed from 578b8144d44d00316b107eafd915c60ebecb604e to dc917d4e81a9f7daf8560748c0747371440d75d8

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

dc917d4trac 12879 implement a correct element

comment:19 Changed 2 years ago by chapoton

  • Status changed from needs_work to needs_review

done, please review.


New commits:

dc917d4trac 12879 implement a correct element

comment:20 Changed 2 years ago by mderickx

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:

dc917d4trac 12879 implement a correct element

comment:21 Changed 2 years ago by chapoton

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 ?

Last edited 2 years ago by chapoton (previous) (diff)

comment:22 Changed 2 years ago by git

  • Commit changed from dc917d4e81a9f7daf8560748c0747371440d75d8 to 1ea2824e6813a8f2bcaaae332537681126aded03

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

1ea2824trac 12879 stronger check for scalar elements

comment:23 Changed 2 years ago by vbraun

  • Milestone changed from sage-8.1 to sage-8.2

comment:24 Changed 2 years ago by tscrim

@vbraun Why the bump to 8.2?

comment:25 Changed 2 years ago by mderickx

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 2 years ago by mderickx

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 2 years ago by tscrim

@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 2 years ago by chapoton

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 2 years ago by mderickx

Is it possible to get more verbose output?

comment:30 Changed 2 years ago by chapoton

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

comment:31 Changed 2 years ago by tscrim

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 2 years ago by git

  • Commit changed from 1ea2824e6813a8f2bcaaae332537681126aded03 to 761ac1d2b925153462e3529a374abedb8a8ccb07

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

e7132eeMerge branch 'u/chapoton/12879' in 8.1.b2
761ac1dtrac 12879 adding TesTsuite

comment:33 Changed 2 years ago by chapoton

done

comment:34 Changed 2 years ago by tscrim

  • Milestone changed from sage-8.2 to sage-8.1
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Maarten Derickx
  • Status changed from needs_review to positive_review

Thank you.

comment:35 Changed 2 years ago by vbraun

  • Branch changed from u/chapoton/12879 to 761ac1d2b925153462e3529a374abedb8a8ccb07
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.