Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#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 nthiery)

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 nthiery

  • Branch set to u/nthiery/simplify_testsuite_s_some_elements_role_and_logic__no_random_sampling_

comment:2 Changed 5 years ago by nthiery

  • Commit set to 2486f804d260370ee1d9a7a9b3a7962a36ea0e65
  • Status changed from new to needs_review

New commits:

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

comment:3 Changed 5 years ago by nthiery

  • Cc roed saraedum added
  • Description modified (diff)

comment:4 Changed 5 years ago by nthiery

  • Description modified (diff)

comment:5 Changed 5 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM (also with #15919).

comment:6 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 5 years ago by vbraun

  • 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 roed

  • Commit 2486f804d260370ee1d9a7a9b3a7962a36ea0e65 deleted

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

Note: See TracTickets for help on using tickets.