#16244 closed defect (fixed)
Simplify TestSuite's some_elements role and logic (no random sampling)
Reported by: | nthiery | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.3 |
Component: | doctest framework | Keywords: | |
Cc: | mmezzarobba, sage-combinat, roed, saraedum | Merged in: | |
Authors: | Nicolas M. Thiéry | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 2486f80 (Commits) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
Since #14284, TestSuite
(more precisely InstanceTester.some_elements
) tries to be fancy by choosing "some elements" using a random sample. The random sample is built using Python's random.sample
, which requires its input to be a Sequence (i.e. the i-th element can be fetched with o[i]), or some dict-like object. This can get brittle with inputs where __getitem__
is used for other purposes, or where unranking is just computationally expensive. The some_elements
method also assumes __len__
to be implemented and cheap enough.
Example:
sage: FF = IntegerModRing(29) # needs to be >21 otherwise random.sample uses a different strategy sage: tester = FF._tester(elements=FF, max_runs=5) sage: list(tester.some_elements()) ... ValueError: first letter of variable name must be a letter
This ticket reduces the role of InstanceTester.some_elements
to just do some argument parsing and ensure that at most "max_run" elements are returned. It only requires the input to be iterable.
If we want to have fancy random samples, we should define a specific protocol (typically P.sample()) for it, or just let parents decide on the strategy by defining some_elements appropriately.
This was originaly analyzed in #15919.
TODO: decide whether InstanceTester?.some_elements should return a list or an iterator. A list would be more user friendly (though that's not critical since this is pretty low level) and maybe less error-prone (if a _test_method attempt to reuse the result twice). On the other hand, in case of failure of a _test_method, using an iterator would waste a bit less time before the failure occurs (no need to build all the elements).
Change History (8)
comment:1 Changed 5 years ago by
- Branch set to u/nthiery/simplify_testsuite_s_some_elements_role_and_logic__no_random_sampling_
comment:2 Changed 5 years ago by
- Commit set to 2486f804d260370ee1d9a7a9b3a7962a36ea0e65
- Status changed from new to needs_review
comment:3 Changed 5 years ago by
- Cc roed saraedum added
- Description modified (diff)
comment:4 Changed 5 years ago by
- Description modified (diff)
comment:5 Changed 5 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM (also with #15919).
comment:6 Changed 5 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:7 Changed 5 years ago by
- Branch changed from u/nthiery/simplify_testsuite_s_some_elements_role_and_logic__no_random_sampling_ to 2486f804d260370ee1d9a7a9b3a7962a36ea0e65
- Resolution set to fixed
- Status changed from positive_review to closed
comment:8 Changed 2 years ago by
- Commit 2486f804d260370ee1d9a7a9b3a7962a36ea0e65 deleted
See #23724 for adding this back in (but optionally)
New commits:
#16244: Simplify the role and logic of TestSuite's some_elements (no random sampling)