Opened 11 years ago
Closed 11 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: |
Description (last modified by )
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)
Change History (12)
comment:1 Changed 11 years ago by
- Description modified (diff)
- Status changed from new to needs_work
comment:2 Changed 11 years ago by
- Description modified (diff)
comment:3 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:4 follow-up: ↓ 5 Changed 11 years ago by
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 11 years ago by
- 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 11 years ago by
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 11 years ago by
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: ↓ 10 Changed 11 years ago by
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 11 years ago by
comment:9 Changed 11 years ago by
- 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 11 years ago by
- 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 11 years ago by
- Merged in set to sage-4.6.2.rc1
- Resolution set to fixed
- Status changed from positive_review to closed
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:
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