Opened 10 years ago

Closed 10 years ago

#10712 closed enhancement (fixed)

Mark doctests # long time

Reported by: jdemeyer Owned by: mvngu
Priority: minor Milestone: sage-4.6.2
Component: doctest coverage Keywords:
Cc: Merged in: sage-4.6.2.rc1
Authors: Jeroen Demeyer Reviewers: Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Some doctests take a long time, but are not marked # long time. This ticket will fix the worst offenders (as a goal, all tests exceeding 10 seconds on sage.math.washington.edu).

Sometimes, we can also restructure the doctest to make it take less time without essentially changing the test.

Attachments (1)

10712_long_time.patch (52.8 KB) - added by jdemeyer 10 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from new to needs_work

comment:2 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:4 follow-up: Changed 10 years ago by rbeezer

Hi Jeroen,

These look good to me in principle and execution. I can't vouch for all the mathematics affected, but maybe no one reviewer could? I'll run tests overnight. In the meantime two questions/comments:

  • I can see the times given being meaningless to anybody a few years in the future. 8 seconds? When and where? (Especially if in 3 years Moore's Law has it running in 2 seconds on new hardware.) Would it be easy to say "# longtime (8s on sage.math, 2011)"? Could you do that to the patch with a bit of grep/sed magic?
  • sage/schemes/elliptic_curves/sha_tate.py had two changes I was unsure about. First, a test related to #10096 is deleted. Is there a reason? Second, some tests ("supersingular cases") are claimed to be the "easiest" possible and a comment to that effect is being deleted. I'd even suggest with al the time savings, the rank 1 test could be sent to the "short time" category? Not everybody runs long tests, so it'd be good to catch as much possible with the standard suite, and this sounds like a good candidate.

Great job cleaning up the tests, and the code in places! I'm amazed none of my tests got caught up in this.

You didn't like "oyoyoy a bug" for an assert statement?? ;-)

Rob

comment:5 in reply to: ↑ 4 ; follow-up: Changed 10 years ago by jdemeyer

  • Reviewers set to Rob Beezer
  • Status changed from needs_review to needs_work

Replying to rbeezer:

  • I can see the times given being meaningless to anybody a few years in the future. 8 seconds? When and where? (Especially if in 3 years Moore's Law has it running in 2 seconds on new hardware.) Would it be easy to say "# longtime (8s on sage.math, 2011)"? Could you do that to the patch with a bit of grep/sed magic?

I could, yes.

  • sage/schemes/elliptic_curves/sha_tate.py had two changes I was unsure about. First, a test related to #10096 is deleted. Is there a reason?

The same test appears twice in the same file, exactly the same.

Second, some tests ("supersingular cases") are claimed to be the "easiest" possible and a comment to that effect is being deleted.

You mean this:

# note on the doctests (cw 2010) 
# these tests are among the easiest test possible,  
# so even if the rank 1 case in supersingular that 
# takes more than 20 sec, should not be excluded. 

Well, that comment doesn't make any sense to me and it's also grammatically wrong. If you think you understand what it means, feel free to rewrite it.

I'd even suggest with al the time savings, the rank 1 test could be sent to the "short time" category? Not everybody runs long tests, so it'd be good to catch as much possible with the standard suite, and this sounds like a good candidate.

Fine.

comment:6 in reply to: ↑ 5 Changed 10 years ago by rbeezer

Replying to jdemeyer:

The same test appears twice in the same file, exactly the same.

Thanks - didn't look carefully enough to see that.

Well, that comment doesn't make any sense to me and it's also grammatically wrong. If you think you understand what it means, feel free to rewrite it.

I understood this to mean the "rank 1 supersingular" case was the minimum non-trivial example to fully exercise the code, even if it ran for a long time. Thus the plea not to delete it. I'll craft a rewrite in the morning.

All tests passed (long and "short") and gained a significant speed-up.

Rob

comment:7 Changed 10 years ago by rbeezer

Here's a rewrite on those comments. I thought it would be easier to just post it here for your use rather than mucking around with a patch. Let me know if a patch would be better for you.

# note on the doctests (cw 2010)
# These tests are among the simplest possible.
# So even if the supersingular rank 1 test may
# need to run for a long time, it should not
# be excluded.

I got about a 7% speedup running long tests - maybe due to some really long tests that got shortened? And short-test-time to long-test-time is now about a 3:4 ratio on my machine, which sounds good to me.

If you are willing to expand the description of actual times on tests (as suggested above), then I'm ready to give this a positive review.

comment:8 follow-up: Changed 10 years ago by jdemeyer

I'm still not convinced by the usefulness of the "note on the doctests". I mean, such a note could be added to every doctest: "don't delete this test". I think it's obvious that a test is there for a reason and that it should not simply be deleted. The test also doesn't take an unusually long time, 10 to 20 seconds is fine for a "# long time" test.

Changed 10 years ago by jdemeyer

comment:9 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Updated version. I removed "# long time" from a few doctests in devel/sage/sage/schemes/elliptic_curves/sha_tate.py.

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

  • Status changed from needs_review to positive_review

Replying to jdemeyer:

I'm still not convinced by the usefulness of the "note on the doctests".

OK, point taken.

Updated patch passes all long tests. Thanks for taking the time to improve all this!

comment:11 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.6.2.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.