Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

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

trac_6070_new.patch (58.2 KB) - added by davidloeffler 10 years ago.
replaces both previous patches
trac_6070_workaround.patch (3.2 KB) - added by davidloeffler 10 years ago.
apply over previous patch

Download all attachments as: .zip

Change History (17)

comment:1 Changed 10 years ago by davidloeffler

  • 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%

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.

comment:2 Changed 10 years ago by cremona

  • 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 davidloeffler

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 davidloeffler

  • 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 cremona

  • 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 mabshoff

  • 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

Changed 10 years ago by davidloeffler

replaces both previous patches

comment:7 Changed 10 years ago by davidloeffler

  • 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: Changed 10 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 10 years ago by mabshoff

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

Changed 10 years ago by davidloeffler

apply over previous patch

comment:10 Changed 10 years ago by davidloeffler

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 cremona

  • 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 davidloeffler

(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 mabshoff

  • 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 mabshoff

  • 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 davidloeffler

  • Authors set to David Loeffler
  • Merged in set to 4.0.rc0
  • Reviewers set to John Cremona
Note: See TracTickets for help on using tickets.