#5736: [with patch, needs work] Improve doctest coverage for sage/modular/hecke
---------------------------+------------------------------------------------
Reporter: davidloeffler | Owner: davidloeffler
Type: defect | Status: assigned
Priority: major | Milestone: sage-3.4.2
Component: modular forms | Keywords:
---------------------------+------------------------------------------------
Comment(by was):
REFEREE REPORT:
1. Where you have
{{{
623 elif self.__ambient != other.__ambient:
624 return cmp(self.__ambient, other.__ambient)
}}}
I would typically write
{{{
c = cmp(self.__ambient, other.__ambient)
if c: return c
}}}
since that entails only one comparison instead of 2.
2. This code makes me nervous
{{{
86 verbose("Trying to copy over
precomputed matrix...")
87 y._HeckeOperator__matrix =
x._HeckeOperator__matrix
88 verbose("...Succeeded")
}}}
One worry is that the basis might be different. I think there is nothing
a priori to guarantee that the basis are the same for two generic Hecke
modules that compare as equal, so one should check not only that the
parents are equal but that they have the same basis. Second, why use the
hidden _HeckeOperator__matrix attribute? Could you just use the .matrix()
method (assuming there is one)? A similar comment also applies about
basis in the next elif.
3. In hecke_operator.py there is a spurious period in line 55 of your
patch:
{{{
54 Return True if x is of type HeckeAlgebraElement.
55 .
56 EXAMPLES::
}}}
4. In hecke_operator.py, there is a TypeError, which should be a
RuntimeError:
{{{
455 else:
456 raise TypeError, "Bug in coercion code" #
can't get here.
}}}
It should be RuntimeError, since it's reporting a bug, so shouldn't just
silently get absorbed by the coercion models exception catching.
5. Here, I wonder if we could remove the 5x5 constraint, since now
matrices have their own (I think 20x20) cutoff for printing:
{{{
347 459 def _repr_(self):
460 r"""
461 String representation of self. The matrix is not
printed if the number
462 of rows or columns is > 5.
463
464 EXAMPLES::
465
466 sage: M = ModularSymbols(1,12)
467 sage:
M.hecke_operator(2).matrix_form()._repr_()
468 'Hecke operator on Modular Symbols space of
dimension 3 for Gamma_0(1) of weight 12 with sign 0 over Rational Field
defined by:\n[ -24 0 0]\n[ 0 -24 0]\n[4860 0 2049]'
469 """
348 470 if
max(self.__matrix.nrows(),self.__matrix.ncols()) > 5:
349 471 mat = "(not printing %s x %s
matrix)"%(self.__matrix.nrows(), self.__matrix.ncols())
}}}
6. I guess this should be RuntimeError too?
{{{
578 else:
579 raise TypeError, "Bug in coercion code" #
can't get here
}}}
7. A doctest for the __hash__() method is ''wrong'':
{{{
340 514 def __hash__(self):
341 return hash((self.__weight, self.level(),
self.base_ring(), str(self)))
515 r"""
516 Return a hash of self.
517
518 EXAMPLES::
519
520 sage: ModularSymbols(22).__hash__ # random
521 """
522 return hash((self.__weight, self.level(),
self.base_ring()))
}}}
Note above you just print the function, not its value. Replace by
{{{.__hash__()}}}
Many thanks for doing such an amazing job documented all this code!!
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/5736#comment:3>
Sage <http://sagemath.org/>
Sage - Open Source Mathematical Software: Building the Car Instead of
Reinventing the Wheel
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en
-~----------~----~----~----~------~----~------~--~---
