Opened 7 years ago

Closed 7 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 roed)

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

  1. 14284.patch
  2. trac_14284_review.patch
  3. trac_14284_review_review.patch
  4. trac_14284_fix.patch

Attachments (4)

14284.patch (11.6 KB) - added by roed 7 years ago.
trac_14284_review.patch (6.3 KB) - added by roed 7 years ago.
trac_14284_review_review.patch (5.3 KB) - added by saraedum 7 years ago.
trac_14284_fix.patch (780 bytes) - added by saraedum 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by roed

  • Dependencies set to #14285

comment:2 Changed 7 years ago by roed

  • Authors set to David Roe
  • Status changed from new to needs_review

comment:3 Changed 7 years ago by saraedum

  • Status changed from needs_review to needs_work

There is a problem with short lists.

comment:4 Changed 7 years ago by saraedum

  • Reviewers set to Julian Rueth

comment:5 Changed 7 years ago by roed

  • Status changed from needs_work to needs_review

Fixed the problem with nth root and with unrank not working correctly.

Changed 7 years ago by roed

comment:6 Changed 7 years ago by roed

See #14293 for a bug revealed by this ticket.

Changed 7 years ago by roed

comment:7 Changed 7 years ago by saraedum

  • Description modified (diff)

comment:8 Changed 7 years ago by saraedum

  • Status changed from needs_review to positive_review

comment:9 follow-up: Changed 7 years ago by nthiery

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: Changed 7 years ago by roed

  • 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 7 years ago by nthiery

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 7 years ago by saraedum

comment:12 Changed 7 years ago by saraedum

  • Status changed from needs_work to needs_review

Changed 7 years ago by saraedum

comment:13 Changed 7 years ago by roed

  • Description modified (diff)

comment:14 Changed 7 years ago by saraedum

  • Status changed from needs_review to positive_review

[I uploaded the patches, but they were actually written by David Roe]

comment:15 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.9.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.