Opened 9 years ago

Closed 9 years ago

Last modified 5 years ago

#16244 closed defect (fixed)

Simplify TestSuite's some_elements role and logic (no random sampling)

Reported by: Nicolas M. Thiéry Owned by:
Priority: major Milestone: sage-6.3
Component: doctest framework Keywords:
Cc: Marc Mezzarobba, Sage Combinat CC user, David Roe, Julian Rüth Merged in:
Authors: Nicolas M. Thiéry Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 2486f80 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Nicolas M. Thiéry)

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.


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 9 years ago by Nicolas M. Thiéry

Branch: u/nthiery/simplify_testsuite_s_some_elements_role_and_logic__no_random_sampling_

comment:2 Changed 9 years ago by Nicolas M. Thiéry

Commit: 2486f804d260370ee1d9a7a9b3a7962a36ea0e65
Status: newneeds_review

New commits:

2486f80#16244: Simplify the role and logic of TestSuite's some_elements (no random sampling)

comment:3 Changed 9 years ago by Nicolas M. Thiéry

Cc: David Roe Julian Rüth added
Description: modified (diff)

comment:4 Changed 9 years ago by Nicolas M. Thiéry

Description: modified (diff)

comment:5 Changed 9 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

LGTM (also with #15919).

comment:6 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:7 Changed 9 years ago by Volker Braun

Branch: u/nthiery/simplify_testsuite_s_some_elements_role_and_logic__no_random_sampling_2486f804d260370ee1d9a7a9b3a7962a36ea0e65
Resolution: fixed
Status: positive_reviewclosed

comment:8 Changed 5 years ago by David Roe

Commit: 2486f804d260370ee1d9a7a9b3a7962a36ea0e65

See #23724 for adding this back in (but optionally)

Note: See TracTickets for help on using tickets.