#5736 closed defect (fixed)
[with patch, positive review] Improve doctest coverage for sage/modular/hecke
Reported by: | davidloeffler | Owned by: | davidloeffler |
---|---|---|---|
Priority: | major | Milestone: | sage-4.0 |
Component: | modular forms | Keywords: | |
Cc: | Merged in: | 4.0.alpha0 | |
Authors: | David Loeffler | Reviewers: | William Stein |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This patch adds many new doctests (mainly in sage/modular/hecke); adds new sections to the reference manual for the modules abvar.abvar_newform, abvar.morphism, abvar.lseries, ssmod.ssmod, and quatalg.brandt; and fixes several small bugs discovered in the process.
Attachments (2)
Change History (28)
comment:1 Changed 14 years ago by
Summary: | Improve doctest coverage for sage/modular/hecke → [with patch, needs work] Improve doctest coverage for sage/modular/hecke |
---|
comment:2 Changed 14 years ago by
Status: | new → assigned |
---|---|
Summary: | [with patch, needs work] Improve doctest coverage for sage/modular/hecke → [with patch, needs review] Improve doctest coverage for sage/modular/hecke |
Oops. Mea culpa. The quotes were added because I was trying to hunt down some funny behaviour in a doctest which I suspected was because the output of the doctest ended in "...", which I suspected conflicted with the use of ... as an abbreviation in example docstring output. It turned out to be something completely different causing the problems, but I forgot to take the quotes out again.
Here is a new patch, without the quotes (and with one small additional change: the tests in bordeaux_2008/generators_for_rings now work, as a consequence of the work I did on find_generators.py, so I've removed the ..skip:: directives in that file.)
comment:3 follow-up: 6 Changed 14 years ago by
Summary: | [with patch, needs review] Improve doctest coverage for sage/modular/hecke → [with patch, needs work] Improve doctest coverage for sage/modular/hecke |
---|
REFEREE REPORT:
- 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.
- 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 _HeckeOperatormatrix attribute? Could you just use the .matrix() method (assuming there is one)? A similar comment also applies about basis in the next elif.
- 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::
- 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.
- 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())
- I guess this should be RuntimeError? too?
578 else: 579 raise TypeError, "Bug in coercion code" # can't get here
- 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!!
comment:4 Changed 14 years ago by
What would you suggest for your point number 2 above? The scenario I had in mind was that one might be somehow using "loads" to read in a Hecke operator that had been precomputed at great expense, and it would make sense to avoid recomputing all that data when comparing it with / adding it to / whatever something else that had been computed afresh. E.g. in one session
sage: H = ModularForms(Gamma0(10), 12).hecke_operator(123456789) sage: H.matrix() # very long! sage: save(H, "./bigheckeop.sobj")
(let's assume this would actually terminate in some reasonable time) and later
sage: H = load("./bigheckeop.sobj") sage: ModularForms(Gamma0(10), 12).hecke_operator(6) + H
At present this will raise a RuntimeError?, which is clearly bad. But doing the computation of the enormous Hecke operator's matrix again from scratch isn't ideal either.
Checking that the bases are "the same" is a bit of a problem because what we really need to do is to check that the basis vectors have the same interpretation as a mathematical objects, rather than just that they're the same ones and zeros in memory. E.g. we might have changed the order in which modular symbol basis vectors were enumerated between versions, and then the second basis vector would still be (0,1,0,..), but it would correspond to a different mathematical object.
It's not a very likely use case, I suppose, so maybe it is safer just to force the recomputation. Let me know what you think, and I'll do a new patch.
comment:5 Changed 14 years ago by
It's not a very likely use case, I suppose, so maybe it is safer just to force the recomputation. Let me know what you think, and I'll do a new patch.
You have some good points. The safest thing I can think of that also "solves" your problem (at least for a sufficiently diligent user!), would be to provide the option to set the n-th Hecke operator from a known matrix. Something like
sage: M = ModularForms(Gamma0(10), 12) sage: M.unsafe_set_hecke_matrix(123456789, mat) sage: M.hecke_operator(123456789).matrix() # instant
The documentation for the function would make the issues with bases clear.
This should be done as part of another patch, imho. It could be useful in a lot of contexts, where for some reason one knows what the n-th hecke matrix is much more efficiently than Sage does, but doesn't want to dive into the guts of Sage's modular forms and code why one has this extra knowledge.
comment:6 Changed 14 years ago by
Replying to was:
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.
Investigation revealed several bugs related to this. If you create two submodules of the same ambient module which are equal as submodules but with different bases, then (since they compare as equal) the factory function caching machinery will return the same object as the Hecke algebra of both of them, despite the fact that this will be using the wrong basis matrix for one of them.
I have fixed this, so the Hecke operators on submodules with custom bases are calculated correctly; and I've made it so the __call__ method of Hecke algebra objects, if asked to convert in a Hecke operator on a submodule that is equal to its own, will check that the basis matrices are the same, and if they're not it will apply the appropriate transformation. There is a long doctest in sage.modular.hecke.algebra.__call__ which checks that this is all OK.
The patch trac_5736-fixes.patch fixes this issue and the other issues 1-7 that you raised above.
David
comment:7 Changed 14 years ago by
Summary: | [with patch, needs work] Improve doctest coverage for sage/modular/hecke → [with patch, needs review] Improve doctest coverage for sage/modular/hecke |
---|
comment:8 Changed 14 years ago by
Summary: | [with patch, needs review] Improve doctest coverage for sage/modular/hecke → [with patch, needs work] Improve doctest coverage for sage/modular/hecke |
---|
Hold it, I accidentally introduced a bug (the doctests for supersingular modules fail).
comment:9 Changed 14 years ago by
Summary: | [with patch, needs work] Improve doctest coverage for sage/modular/hecke → [with patch, needs review] Improve doctest coverage for sage/modular/hecke |
---|
Turns out that the doctests fail as a knock-on effect of #4306: the basis_matrix method fails for SupersingularModules?, and the changes I made to the HeckeAlgebra? factory function exposed that. The tiny patch trac_5736-3.patch adds a workaround.
Changed 14 years ago by
Attachment: | trac_5736-rebase.patch added |
---|
replaces all previous patches, rebased to 3.4.2.alpha0
comment:10 Changed 14 years ago by
I have added a new patch that is equivalent to the three patches above combined, rebased to apply over 3.4.2.alpha0. (Isn't the Mercurial Queues add-on wonderful?)
comment:11 Changed 14 years ago by
I applied this to 3.4.2.alpha0, and did "make ptestlong":
sage -t -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py [181.8 s] ---------------------------------------------------------------------- The following tests failed: sage -t -long devel/sage/sage/modular/modform/space.py # 2 doctests failed sage -t -long devel/sage/sage/modular/hecke/hecke_operator.py # 8 doctests failed sage -t -long devel/sage/sage/modular/hecke/algebra.py # 1 doctests failed sage -t -long devel/sage/sage/modular/hecke/module.py # 14 doctests failed ---------------------------------------------------------------------- Total time for all tests: 387.7 seconds wstein@sage:~/build/sage-3.4.2.alpha0$
comment:12 Changed 14 years ago by
Summary: | [with patch, needs review] Improve doctest coverage for sage/modular/hecke → [with patch, needs work] Improve doctest coverage for sage/modular/hecke |
---|
comment:13 Changed 14 years ago by
I am unable to replicate these failures: it works fine for me both on my laptop (32-bit Linux) and on sage.math.washington.edu. Which system are you using? Could you tell me exactly which doctests are failing, and how?
comment:14 Changed 14 years ago by
Description: | modified (diff) |
---|
William: any movement on this? It would be nice to get this into 4.0 at least (we seem to have missed the 3.4.2 deadline now), but I can't fix the problems you're having if I don't know what's broken. It works for me on 64-bit and 32-bit Linux so I'm guessing you're using a Mac, but I don't have access to a Mac for testing.
(I have several other modular forms fixes + improvements ready that are dependent on this one.)
comment:15 Changed 14 years ago by
Summary: | [with patch, needs work] Improve doctest coverage for sage/modular/hecke → [with patch, needs review] Improve doctest coverage for sage/modular/hecke |
---|
I applied trac_5736-rebase.patch to 3.4.2.rc0 on sage.math and long doctests pass.
William: Any chances you had crap in your tree unrelated to this patch? While it is too late for 3.4.2 this patch raises coverage by 0.2% [aside from all the nice bug fixes :)], so it really ought to go into 4.0.
Changing to "needs review" again.
Cheers,
Michael
comment:16 Changed 14 years ago by
Hmm, skimming the patch:
519 EXAMPLES:: 520 521 sage: ModularSymbols(22).__hash__() # random 522 1471905187
Why on earth would the hash be random (there are two cases in the patch)? I assume there are 32 vs. 64 bit issues (which can be properly dealt with), but a hash should certainly not be random.
Cheers,
Michael
comment:17 Changed 14 years ago by
Summary: | [with patch, needs review] Improve doctest coverage for sage/modular/hecke → [with patch, needs work] Improve doctest coverage for sage/modular/hecke |
---|
Changing back to "needs work" for the hashing issue. Just grep the Sage library tree for either 32-bit
or 64-bit
to see how to create 32 and 64 bit specific output.
Cheers,
Michael
comment:18 Changed 14 years ago by
Another thing: This is wrong since the current LaTeX framework that will be in 3.4.2 uses macros like \QQ
to make the output customizable:
1158 NOTE: The base ring must be `\QQ` or `\ZZ`. 1187 NOTE: The base ring must be `mathbb{Q}` or `mathbb{Z}`.
Cheers,
Michael
comment:19 Changed 14 years ago by
Summary: | [with patch, needs work] Improve doctest coverage for sage/modular/hecke → [with patch, needs review] Improve doctest coverage for sage/modular/hecke |
---|
Here's a tiny patch that addresses the two issues raised by mabshoff.
comment:20 Changed 14 years ago by
Summary: | [with patch, needs review] Improve doctest coverage for sage/modular/hecke → [with patch, needs work] Improve doctest coverage for sage/modular/hecke |
---|
I applied both patches to a clean 3.4.2.rc0 install and got:
make ptestlong ... The following tests failed: sage -t -long devel/sage/sage/modular/modsym/ambient.py # 4 doctests failed sage -t -long devel/sage/sage/tests/book_stein_modform.py # 3 doctests failed sage -t -long devel/sage/sage/modular/modsym/manin_symbols.py # 1 doctests failed sage -t -long devel/sage/doc/en/bordeaux_2008/modular_symbols.rst # 1 doctests failed ---------------------------------------------------------------------- Total time for all tests: 312.6 seconds wstein@sage:~/build/sage-3.4.2.rc0$
comment:21 Changed 14 years ago by
Ok, I was surprised this patch did break things again, but there are two issues here:
An extra space has been inserted when printing the basis, for example:
File "/scratch/wstein/build/sage-3.4.2.rc0/devel/sage-0/sage/tests/book_stein_modform.py", line 70: : M.basis() Expected: ({Infinity,0}, {-1/8,0}, {-1/9,0}) Got: ({Infinity, 0}, {-1/8, 0}, {-1/9, 0})
In addition the printing order for modular_symbol_rep() is reversed:
age -t -long devel/sage/doc/en/bordeaux_2008/modular_symbols.rst ********************************************************************** File "/scratch/wstein/build/sage-3.4.2.rc0/devel/sage-0/doc/en/bordeaux_2008/modular_symbols.rst", line 124: sage: a.modular_symbol_rep() Expected: 36*X^2*{-1/6,0} + 12*X*Y*{-1/6,0} + Y^2*{-1/6,0} Got: Y^2*{-1/6, 0} + 12*X*Y*{-1/6, 0} + 36*X^2*{-1/6, 0} **********************************************************************
So all those four doctests are simple to fix.
Cheers,
Michael
comment:22 Changed 14 years ago by
Summary: | [with patch, needs work] Improve doctest coverage for sage/modular/hecke → [with patch, needs review] Improve doctest coverage for sage/modular/hecke |
---|
Now the truly puzzling thing is this:
- The above failures are in William's tree on sage.math - I ran them there.
- When I take those two patches and import them into my clean 3.4.2.rc0 tree in /scratch on sage.math all doctests passs.
William's testing tree seems to be a clone from a tree that has the following patches in sage-main:
changeset: 12151:e0257aa492ab tag: tip user: William Stein <wstein@gmail.com> date: Sat May 02 22:56:24 2009 -0700 summary: trac #5968 -- part 2 -- increase doctest coverage of sage/modular/modsym/modular_symbols.py from 0% to 100% changeset: 12150:673cc9b82a7d user: William Stein <wstein@gmail.com> date: Sat May 02 22:46:52 2009 -0700 summary: trac 5968 -- increase doctest coverage of sage/modular/modsym/modular_symbols.py from 0% to 100% changeset: 12149:901f4946fd7e user: William Stein <wstein@gmail.com> date: Sat May 02 02:15:34 2009 -0700 summary: trac #5882 -- part 4 -- mop up some issues with overzealous changes to matrix_morphism. changeset: 12148:312499531bcb user: William Stein <wstein@gmail.com> date: Sat May 02 01:31:38 2009 -0700 summary: trac #5882 -- part 3 of "implement general package for finitely generated not-necessarily free R-modules" changeset: 12147:82b9ade5605a user: William Stein <wstein@gmail.com> date: Sat May 02 01:48:24 2009 -0700 summary: trac #5882 (part 2) -- implement general package for finitely generated not-necessarily free R-modules changeset: 12146:6023d9680d5e user: William Stein <wstein@gmail.com> date: Thu Apr 30 13:30:21 2009 -0700 summary: trac #5882 -- part 1 of implementation of finitely generated modules over a PID changeset: 12145:9ee83eda286e user: William Stein <wstein@gmail.com> date: Sat May 02 01:38:52 2009 -0700 summary: trac #5887 -- major bugs in morphisms of R-modules (rebased against 3.4.2.rc0)
So I assume that his build directory contains crap from those patches that cause these failures that a sage -b
does not fix since there are no dependencies when cloning from the rc0 revision. A sage -ba
while using the sage-0 clone would clear this up, but since this isn't my tree I will not do so :)
In the end this patch should be reviewed on a truly clean tree, so I am changing this patch to "needs review".
Thoughts?
Cheers,
Michael
comment:23 Changed 14 years ago by
Indeed, this was exactly caused by this issue mentioned above:
wstein@sage:~/build/sage-3.4.2.rc0$ ./sage -sh Starting subshell with Sage environment variables set. Be sure to exit when you are done and do not do anything with other copies of Sage! wstein@sage:~/build/sage-3.4.2.rc0$ cd devel/sage wstein@sage:~/build/sage-3.4.2.rc0/devel/sage$ touch sage/modular/modsym/modular_symbols.py wstein@sage:~/build/sage-3.4.2.rc0/devel/sage$ cd ../.. wstein@sage:~/build/sage-3.4.2.rc0$ ./sage -b ---------------------------------------------------------- sage: Building and installing modified Sage library files. Installing c_lib scons: `install' is up to date. Updating Cython code.... Time to execute 0 commands: 1.38282775879e-05 seconds Finished compiling Cython code (time = 0.30867099762 seconds) running install running build running build_py copying sage/modular/modsym/modular_symbols.py -> build/lib.linux-x86_64-2.5/sage/modular/modsym running build_ext running build_scripts running install_lib copying build/lib.linux-x86_64-2.5/sage/modular/modsym/modular_symbols.py -> /scratch/wstein/build/sage-3.4.2.rc0/local/lib/python2.5/site-packages/sage/modular/modsym byte-compiling /scratch/wstein/build/sage-3.4.2.rc0/local/lib/python2.5/site-packages/sage/modular/modsym/modular_symbols.py to modular_symbols.pyc running install_scripts changing mode of /scratch/wstein/build/sage-3.4.2.rc0/local/bin/spkg-debian-maybe to 755 running install_egg_info Removing /scratch/wstein/build/sage-3.4.2.rc0/local/lib/python2.5/site-packages/sage-0.0.0-py2.5.egg-info Writing /scratch/wstein/build/sage-3.4.2.rc0/local/lib/python2.5/site-packages/sage-0.0.0-py2.5.egg-info real 0m1.006s user 0m0.810s sys 0m0.190s wstein@sage:~/build/sage-3.4.2.rc0$ sage -t -long devel/sage/sage/modular/modsym/ambient.py # 4 doctests failed sage -t -long "devel/sage/sage/modular/modsym/ambient.py" [5.8 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 5.8 seconds wstein@sage:~/build/sage-3.4.2.rc0$ sage -t -long devel/sage/sage/tests/book_stein_modform.py # 3 doctests failedsage -t -long "devel/sage/sage/tests/book_stein_modform.py" [3.3 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 3.3 seconds wstein@sage:~/build/sage-3.4.2.rc0$ sage -t -long devel/sage/sage/modular/modsym/manin_symbols.py # 1 doctests failed sage -t -long "devel/sage/sage/modular/modsym/manin_symbols.py" [1.5 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 1.5 seconds wstein@sage:~/build/sage-3.4.2.rc0$ sage -t -long devel/sage/doc/en/bordeaux_2008/modular_symbols.rst # 1 doctests failed sage -t -long "devel/sage/doc/en/bordeaux_2008/modular_symbols.rst" [1.1 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 1.1 seconds wstein@sage:~/build/sage-3.4.2.rc0$
comment:24 Changed 14 years ago by
Summary: | [with patch, needs review] Improve doctest coverage for sage/modular/hecke → [with patch, positive review] Improve doctest coverage for sage/modular/hecke |
---|
OK, you are right. Positive review, since when reading the patches everything looks good to me. Thanks!!
comment:25 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Merged both patches in Sage 4.0.alpha0.
Cheers,
Michael
comment:26 Changed 14 years ago by
Authors: | → David Loeffler |
---|---|
Merged in: | → 4.0.alpha0 |
Reviewers: | → William Stein |
This patch does cause one doctest failure:
I am not sure why the
'
was added in the first place, but it seems to be non-standard in Sage to add the quotes.William might have the definite POV on this issue :)
Cheers,
Michael