Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nthiery opened this issue Apr 26, 2014 · 10 comments
Closed

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

nthiery opened this issue Apr 26, 2014 · 10 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Apr 26, 2014

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).

CC: @mezzarobba @sagetrac-sage-combinat @roed314 @saraedum

Component: doctest framework

Author: Nicolas M. Thiéry

Branch: 2486f80

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/16244

@nthiery nthiery added this to the sage-6.2 milestone Apr 26, 2014
@nthiery
Copy link
Contributor Author

nthiery commented Apr 26, 2014

@nthiery
Copy link
Contributor Author

nthiery commented Apr 26, 2014

Commit: 2486f80

@nthiery
Copy link
Contributor Author

nthiery commented Apr 26, 2014

New commits:

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

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Apr 28, 2014

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 28, 2014

comment:5

LGTM (also with #15919).

@vbraun
Copy link
Member

vbraun commented May 12, 2014

@roed314
Copy link
Contributor

roed314 commented Aug 26, 2017

comment:8

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

@roed314
Copy link
Contributor

roed314 commented Aug 26, 2017

Changed commit from 2486f80 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants