Ticket #7678 (closed enhancement: fixed)
shorten very long doctests in rings/arith.py
| Reported by: | AlexGhitza | Owned by: | tbd |
|---|---|---|---|
| Priority: | minor | Milestone: | sage-4.3.1 |
| Component: | doctest coverage | Keywords: | |
| Cc: | Work issues: | ||
| Report Upstream: | N/A | Reviewers: | John Cremona |
| Authors: | Alex Ghitza | Merged in: | sage-4.3.1.alpha0 |
| Dependencies: | Stopgaps: |
Description
On sage.math, before the patch:
sage -t -long "devel/sage-main/sage/rings/arith.py"
[162.6 s]
And after the patch:
sage -t -long "devel/sage-main/sage/rings/arith.py"
[50.2 s]
I'm putting the milestone as 4.3 only because this is almost certainly not going to break anything whatsoever. Please change it to 4.3.1 if you think that's more appropriate.
Attachments
-
trac_7678.patch
(1.5 KB) -
added by AlexGhitza 3 years ago.
-
trac_7678.2.patch
(1.6 KB) -
added by AlexGhitza 3 years ago.
Change History
comment:2 follow-up: ↓ 7 Changed 3 years ago by cremona
- Status changed from needs_review to needs_work
- Reviewers set to John Cremona
I see what you did here: (1) you removed tha 'gap' algorithm from the test, presumably because it was slowest, and (2) instead of testing all i in each of the ranges 2 to 2255 and 2256 to 5000 you pick 500 random indices from those ranges and use those.
I don't think that strategy (2) is a good idea, since if this test ever fails it will be hard to find out exactly where (i.e. for which i). The tests in our standard test suite should surely be deterministic. What I did in a similar situation was to once and for all pick a random set of indices, and make the doctest test those indices only. This does not prevent us from having a more strenuous test to do on occasion, I am only proposing limiting what happens every time the whole of Sage is tested.
For that reason (only) I have put this as "needs work", and will post to sage-devel so that others who commented on your observation can come and have their say.
comment:3 Changed 3 years ago by mhansen
All of the random seeds are set at the beginning of the doctest, so it would be deterministic.
comment:4 Changed 3 years ago by cremona
In that case I'm changing this to "positive review". Except that I can't, there is no such button below!
comment:6 Changed 3 years ago by robertwb
- Status changed from needs_review to positive_review
Can't give a positive review until it needs a review.
comment:7 in reply to: ↑ 2 Changed 3 years ago by AlexGhitza
Replying to cremona:
I see what you did here: (1) you removed tha 'gap' algorithm from the test, presumably because it was slowest
Actually, the splitting into range(2, 2255) including 'gap' and range(2256, 5000) excluding 'gap' was there before I touched this, and it was indeed because gap gets rather slow at doing this computation. The only real change I made was to pick 500 integers in each range instead of testing the whole range.
comment:8 Changed 3 years ago by AlexGhitza
- Status changed from positive_review to needs_work
I did something stupid in the first patch though, namely to remove #long time from two doctests that depend on previous long time computations. This of course has no effect on sage -t -long but it breaks sage -t. (Note to self: should test with and without -long in the future.)
Apply only the new patch, which fixes this. And I guess this should technically be reviewed again.
comment:10 Changed 3 years ago by cremona
That's very odd since I definitely ran the test with and without -long. I'll have to do it again.
comment:11 Changed 3 years ago by cremona
- Status changed from needs_review to positive_review
I did it again (eventually) and am now happy to give this a positive review.
comment:12 Changed 3 years ago by mhansen
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-4.3.1.alpha0
