Skip to content

Tests for task's API #29

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 12, 2021
Merged

Tests for task's API #29

merged 6 commits into from
Nov 12, 2021

Conversation

psteinb
Copy link
Contributor

@psteinb psteinb commented Nov 9, 2021

As discussed in #19 this PR isolated the unit test for two_moons to serve as a starting point establishing tests for all tasks.

@jan-matthis
Copy link
Contributor

jan-matthis commented Nov 10, 2021 •
edited
Loading

This is great, cheers!

I'd suggest that we parametrizing those tests such that they run for all tasks, i.e., something along the lines of:

@pytest.mark.parametrize(
    "task_name",
    [
        (task_name,) for task_name in sbibm.get_available_tasks()
    ],
)

What do you think?

@jan-matthis jan-matthis mentioned this pull request Nov 10, 2021
Closed
@psteinb
Copy link
Contributor Author

psteinb commented Nov 10, 2021

I think that this makes a lot of sense. I can add that if you want.
I suggest to be a bit more selective though in terms of what is tested, in order not to extend the runtime of the tests. One way to go would perhaps be, to do these automated tests in /repo/tests/task as they mostly test the interface of the tests which is settled in /repo/sbibm/tasks/task.py.
For tests potentially running longer, we could mirror the same structure as in /repo/sbibm/tasks/ and allow for specialized tests (e.g. see the .demo.test cases in this PR) in the/repo/tests/tasks/two_moons/test_special.py`. I think this keeps the balance between a "god test suite" and more specialized tests if they are needed. And I'd hope, it makes contributing easier.

@jan-matthis
Copy link
Contributor

I think that this makes a lot of sense. I can add that if you want.

That would be great!

I suggest to be a bit more selective though in terms of what is tested, in order not to extend the runtime of the tests. One way to go would perhaps be, to do these automated tests in /repo/tests/task as they mostly test the interface of the tests which is settled in /repo/sbibm/tasks/task.py.
For tests potentially running longer, we could mirror the same structure as in /repo/sbibm/tasks/ and allow for specialized tests (e.g. see the .demo.test cases in this PR) in the/repo/tests/tasks/two_moons/test_special.py`. I think this keeps the balance between a "god test suite" and more specialized tests if they are needed. And I'd hope, it makes contributing easier.

That's an excellent suggestion. I agree that, depending on the task, we will probably want specialized tests as well -- probably mostly marked as slow for CI execution.

psteinb added a commit to psteinb/sbibm that referenced this pull request Nov 11, 2021
@psteinb
removed test of two_moons in favor of sbi-benchmark#29
a676e65
@jan-matthis jan-matthis linked an issue Nov 11, 2021 that may be closed by this pull request
Improvements to unit tests #23
Closed
@psteinb
unit tests for 'all' tests
b7b78f7 
- 3 types of test suites added
  + test_task_interface.py for mere API tests
  + test_task_rej_abc_demo.py for testing the API demonstrated on the
  landing page (README.md)
  + test_task_benchmark.py to see/document if the benchmarks work

- added some "noref" sentinels for tasks which do not have a reference posterior
- using sets to better work with list of tasks to run tests for
- as of now, the tests exclude julia based tests
@psteinb psteinb force-pushed the two-moons-task-test branch from b4f3adb to b7b78f7 Compare November 11, 2021 17:02
@psteinb psteinb marked this pull request as ready for review November 11, 2021 17:03
@psteinb
Copy link
Contributor Author

psteinb commented Nov 11, 2021 •
edited
Loading

@jan-matthis done for now. I am happy to adapt #18 if this can be merged earlier.
Please review whenever you can find the time.

jan-matthis
jan-matthis reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@jan-matthis jan-matthis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I think this provides a good foundation for testing existing and future tasks. Thanks a lot for your work on this, much appreciated!

I only left small comments

@jan-matthis jan-matthis changed the title unit test for two moons, basis for testing other tasks Tests for task's API Nov 12, 2021
@psteinb
removed superfluous code
26d92f5 
- include noref tasks as we don't use the reference posterior
- add TODO for later
@psteinb
using pyro set_rng_seed utility to fix seed in tests
9e91818
@psteinb
Copy link
Contributor Author

psteinb commented Nov 12, 2021

Thanks for the review. I hope I implemented those comments alright.

jan-matthis
jan-matthis reviewed Nov 12, 2021
View reviewed changes
Copy link
Contributor

@jan-matthis jan-matthis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for a single comment this looks good to go from my side, cheers!

@psteinb psteinb force-pushed the two-moons-task-test branch from 9ffc0f6 to abc4a85 Compare November 12, 2021 16:04
@psteinb psteinb requested a review from jan-matthis November 12, 2021 16:05
@jan-matthis jan-matthis merged commit 60c1210 into sbi-benchmark:main Nov 12, 2021
@jan-matthis
Copy link
Contributor

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jan-matthis jan-matthis Awaiting requested review from jan-matthis

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improvements to unit tests
2 participants
@psteinb @jan-matthis