#6070 closed defect (fixed)
[with new patch, with positive review] Get doctest coverage in sage/modular to 100%
Reported by: | davidloeffler | Owned by: | davidloeffler |
---|---|---|---|
Priority: | major | Milestone: | sage-4.0 |
Component: | modular forms | Keywords: | documentation, modular symbols |
Cc: | cremona | Merged in: | 4.0.rc0 |
Authors: | David Loeffler | Reviewers: | John Cremona |
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
This is a follow-up of #6042. That patch increased doctest coverage in the modular subdirectory from 91.8% to 96.4%. I have finished off the job by doctesting the last few files in sage/modular/modsym, and will upload a patch soon (once I have got a ticket number to put in the patch header, and run full tests on 4.0.alpha0).
Attachments (2)
Change History (17)
comment:1 Changed 10 years ago by
- Cc cremona added
- Summary changed from Get doctest coverage in sage/modular to 100% to [with patch, needs review] Get doctest coverage in sage/modular to 100%
comment:2 Changed 10 years ago by
- Summary changed from [with patch, needs review] Get doctest coverage in sage/modular to 100% to [with patch, with positive review] Get doctest coverage in sage/modular to 100%
Great job. Applies fine as advertised and all looks very good.
Small point 1: in the preamble to boundary.py there a re few things nto in math mode which could be (e.g. Gamma1).
Small point 2: is it intended that g1list & ghlist are not included in the reference manual?
I'm giving this a positive review anyway, but if David wants to make further small changes he is welcome.
comment:3 Changed 10 years ago by
I can't help taking the bait. Further patch coming. This would have taken me rather less time if I hadn't uncovered yet another bug in the process (see #6072).
comment:4 Changed 10 years ago by
- Summary changed from [with patch, with positive review] Get doctest coverage in sage/modular to 100% to [with patches, with partial review] Get doctest coverage in sage/modular to 100%
comment:5 Changed 10 years ago by
- Summary changed from [with patches, with partial review] Get doctest coverage in sage/modular to 100% to [with patches, with positive review] Get doctest coverage in sage/modular to 100%
It looks fine to me ( and I hardly dare suggesting anything else new or David would write a whole book!).
Extra patch applies fine on top of old, and builds (inc. reference html) fine. And it looks very good (including the two new files now in the ref man).
comment:6 Changed 10 years ago by
- Summary changed from [with patches, with positive review] Get doctest coverage in sage/modular to 100% to [with patches, needs work] Get doctest coverage in sage/modular to 100%
This patch by itself causes a doctest failure for the pickle jar:
sage -t -long "devel/sage/sage/structure/sage_object.pyx" ********************************************************************** File "/scratch/mabshoff/sage-4.0.rc0/devel/sage/sage/structure/sage_object.pyx", line 724: sage: sage.structure.sage_object.unpickle_all(std) Expected: doctest:...: DeprecationWarning: RQDF is deprecated; use RealField(212) instead. Successfully unpickled 483 objects. Failed to unpickle 0 objects. Got: ** failed: _class__sage_modular_modform_cuspidal_submodule_CuspidalSubmodule_g1_Q__.sobj doctest:1172: DeprecationWarning: RQDF is deprecated; use RealField(212) instead. Failed: _class__sage_modular_modform_cuspidal_submodule_CuspidalSubmodule_g1_Q__.sobj Successfully unpickled 482 objects. Failed to unpickle 1 objects. ********************************************************************** 1 items had failures: 1 of 7 in __main__.example_16 ***Test Failed*** 1 failures. For whitespace errors, see the file /scratch/mabshoff/sage-4.0.rc0/tmp/.doctest_sage_object.py [3.2 s] exit code: 1024
That is without #5080, so there is also the other two failures mentioned above. Note the comment on #5080 causing a significant slowdown in sage/schemes/elliptic_curves/sha_tate.py.
Sorry, but "needs work" :(
Cheers,
Michael
comment:7 Changed 10 years ago by
- Summary changed from [with patches, needs work] Get doctest coverage in sage/modular to 100% to [with new patch, needs review] Get doctest coverage in sage/modular to 100%
That will be because I promoted the G1list and GHlist classes from plain Python classes to Sage objects, which apparently breaks unpickling. There wasn't any particular reason to do this anyway -- it just seemed neater. So here is a new patch that doesn't do this.
comment:8 follow-up: ↓ 9 Changed 10 years ago by
Since I gave the earlier patch a positive review I clearly don't know all the tests which need doing -- so Michael, is David's new patch passes your tests it is certainly ok with me.
comment:9 in reply to: ↑ 8 Changed 10 years ago by
Replying to cremona:
Since I gave the earlier patch a positive review I clearly don't know all the tests which need doing -- so Michael, is David's new patch passes your tests it is certainly ok with me.
I haven't tried the new patch yet, but the old one caused issues in sage/modular/modsym/space.py
which were fixed by #5080. Unfortunately that ticket caused a massive slowdown (see David's comment toward the end why), so I cannot merge both tickets due to the slowdown and this ticket due to the failure.
We are about to leave for MSR, so I won't have net access for the next 8 hours or so.
Cheers,
Michael
comment:10 Changed 10 years ago by
Right, well, here's a temporary solution. The reason for the doctest failures in modsym/space.py
without #5080 was because #5080 changed the behaviour of dual_free_module
slightly when the ambient space had sign 0 but the given subspace had fixed sign. The new doctests I added to {{{space.py}} relied on this changed behaviour. The new patch I've just uploaded makes a trivial change to these doctests so that they pass without having #5080 applied. So we can get this merged now, without it having to wait for me (or anyone else) to get around to fixing the speed regression at #5080.
How does that sound?
David
comment:11 Changed 10 years ago by
- Summary changed from [with new patch, needs review] Get doctest coverage in sage/modular to 100% to [with new patch, with positive review] Get doctest coverage in sage/modular to 100%
Sounds good as a reasonable stopgap.
I applied both patches in turn to 4.0.alpha0. There was this:
patching file sage/modular/modsym/ambient.py Hunk #1 succeeded at 233 with fuzz 1 (offset -81 lines).
which I think can be ignored. All tests (including long) in sage/modular pass, as does Michael's test of sage/structure/sage_object.pyx. So I am reinstating the positive review and hoping for the best.
comment:12 Changed 10 years ago by
(FWIW: That fuzz can certainly be safely ignored, as it comes from the fact that I cut out the patch on top of #6042, which is already merged.)
comment:13 Changed 10 years ago by
- Milestone changed from sage-4.0.1 to sage-4.0
With both patches applied all tests including the pickle jar pass. The speed regression due to #5080 is avoided since the doctest has been adjusted to not hit the bug, so we are good to go.
Cheers,
Michael
comment:14 Changed 10 years ago by
- Resolution set to fixed
- Status changed from new to closed
Merged both patches in Sage 4.0.rc0.
Cheers,
Michael
comment:15 Changed 10 years ago by
- Merged in set to 4.0.rc0
- Reviewers set to John Cremona
Here's a patch. The description has a typo; you're not meant to apply it over itself :-) It should say "apply over #6042 and #5080". (Without #5080 the patch will still apply, but two doctests in
sage/modular/modsym/space.py
will fail.)In the course of doctesting the latex output functions for modular symbols, I found a bug in
sage/misc/latex.py
(it omits plus signs when latexing a formal linear combination if all the coefficients are 1). So that is also fixed in the above patch.