Opened 9 years ago
Closed 9 years ago
#14284 closed enhancement (fixed)
Sampling in unit tests
Reported by: | roed | Owned by: | jason |
---|---|---|---|
Priority: | major | Milestone: | sage-5.9 |
Component: | misc | Keywords: | |
Cc: | Merged in: | sage-5.9.beta2 | |
Authors: | David Roe | Reviewers: | Julian Rueth |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14285 | Stopgaps: |
Description (last modified by )
In writing unit tests (e.g. _test_associativity
) it can be useful to pass in lots of elements to test. But some tests scale linearly while others scale cubically, so it's not practical to have the same list of elements for all tests. This patch adds a max_runs
option for TestSuite.run
that forces sampling of the element list when a large list is specified.
Apply
Attachments (4)
Change History (19)
comment:1 Changed 9 years ago by
- Dependencies set to #14285
comment:2 Changed 9 years ago by
- Status changed from new to needs_review
comment:3 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 9 years ago by
- Reviewers set to Julian Rueth
comment:5 Changed 9 years ago by
- Status changed from needs_work to needs_review
Fixed the problem with nth root and with unrank not working correctly.
Changed 9 years ago by
comment:6 Changed 9 years ago by
See #14293 for a bug revealed by this ticket.
Changed 9 years ago by
comment:7 Changed 9 years ago by
- Description modified (diff)
comment:8 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:9 follow-up: ↓ 10 Changed 9 years ago by
Hi!
Sorry to jump in a bit late in the discussion; I had not noticed this ticket before.
I definitely see and approve the point of the ticket. On the other hand, I find the current idiom to be used in _test_associativity
and friends a bit heavy. What about an idiom like:
S = tester.some_elements() for x,y,z in tester.some_elements(CartesianProduct(S,S,S)): ...
It's short, and encapsulate TestSuite?'s inner logic for testing strategies (on how many elements to run the tests, whether to do tests at random or not, ...).
This of course requires implementing:
tester.some_elements(XXX)
The default implementation could be to run XXX.some_elements(). Or iterate through XXX, stopping if there are more than n_max elements. Or take a sample if XXX implements sample. Or ...
What do you think? If you agree, then I would suggest doing the changes in this ticket, in order to minimize changes and counter changes (they probably will require some rebasing of my upcoming category patches; the less rebasing the better :-)).
By the way: for reproducibility of test failures, I am not super comfortable with random testing as a default; but that might be just me.
Cheers,
Nicolas
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 9 years ago by
- Status changed from positive_review to needs_work
Replying to nthiery:
Hi!
Sorry to jump in a bit late in the discussion; I had not noticed this ticket before.
Well, we did just open it a couple days ago (Julian's visiting me in Calgary so we're working in person).
I definitely see and approve the point of the ticket. On the other hand, I find the current idiom to be used in
_test_associativity
and friends a bit heavy. What about an idiom like:S = tester.some_elements() for x,y,z in tester.some_elements(CartesianProduct(S,S,S)): ...It's short, and encapsulate TestSuite?'s inner logic for testing strategies (on how many elements to run the tests, whether to do tests at random or not, ...).
This of course requires implementing:
tester.some_elements(XXX)The default implementation could be to run XXX.some_elements(). Or iterate through XXX, stopping if there are more than n_max elements. Or take a sample if XXX implements sample. Or ...
What do you think? If you agree, then I would suggest doing the changes in this ticket, in order to minimize changes and counter changes (they probably will require some rebasing of my upcoming category patches; the less rebasing the better :-)).
Sounds okay to me: I'll make a new version. There will be a couple tests that can't use this idiom (_test_eq_symmetric
for example), but they can certainly use the current approach.
By the way: for reproducibility of test failures, I am not super comfortable with random testing as a default; but that might be just me.
Since Sage sets the random seed at the beginning of each run, tests that use randomness should be reproducible....
comment:11 in reply to: ↑ 10 Changed 9 years ago by
Replying to roed:
Sounds okay to me: I'll make a new version. There will be a couple tests that can't use this idiom (
_test_eq_symmetric
for example), but they can certainly use the current approach.
Thanks!
By the way: there are quite a few spots where _test_associativity is disabled, precisely because they are too expensive (either due to too many elements, but also in some cases because of high degree calculations). See:
grep -r _test_associativity . | grep skip
You might want to play around with those and maybe fix a couple if things work better now.
Since Sage sets the random seed at the beginning of each run, tests that use randomness should be reproducible....
Unless some edit elsewhere in the file changes the order in which things are run. Or if one wants to reproduce an error in the terminal (hopefully not so critical with the upcoming doctest framework if one can finally explore a failing test with the debugger).
But I see the advantages too.
Changed 9 years ago by
comment:12 Changed 9 years ago by
- Status changed from needs_work to needs_review
Changed 9 years ago by
comment:13 Changed 9 years ago by
- Description modified (diff)
comment:14 Changed 9 years ago by
- Status changed from needs_review to positive_review
[I uploaded the patches, but they were actually written by David Roe]
comment:15 Changed 9 years ago by
- Merged in set to sage-5.9.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
There is a problem with short lists.