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 tests suite, remove spy_phase and related functions #886

Merged
merged 59 commits into from
May 13, 2022

Conversation

lebrice
Copy link
Collaborator

@lebrice lebrice commented Apr 14, 2022

Description

This PR addresses #884 , by removing the pretty "spy_phase" stuff from the test suite. This makes tests a lot more transparent.

Changes

  • All tests of a BaseAlgorithmTests sublass are now parametrized to run through each of the different test phases.
    • A new first_phase_only decorator can be used to disable this behaviour.
    • Running tests in all phases has actually been useful: I've identified a few subtle bugs related to using set_state and get_state after the initial number of trials have been observed.
  • a new phase fixture can be requested to get the current TestPhase.
  • BaseAlgoTests.create_algo now creates the algo, and makes it observe the right number of trials for the current phase before returning it.

This PR also fixes the bugs that were highlighted by this change in the test suite, for all the algorithms.

Tests

  • I added corresponding tests for bug fixes and new features. If possible, the tests fail without the changes
  • All new and existing tests are passing ($ tox -e py38; replace 38 by your Python version if necessary)

Documentation

  • I have updated the relevant documentation related to my changes

Quality

  • I have read the CONTRIBUTING doc
  • My commits messages follow this format
  • My code follows the style guidelines ($ tox -e lint)

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: lebrice <fabrice.normandin@gmail.com>
Signed-off-by: lebrice <fabrice.normandin@gmail.com>
Signed-off-by: lebrice <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice lebrice marked this pull request as ready for review April 14, 2022 20:12
src/orion/algo/pbt/pbt.py Outdated Show resolved Hide resolved
src/orion/testing/algo.py Outdated Show resolved Hide resolved
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
This simplifies the test_state_dict tests to only check if the state can
be transfered from one phase to the next, instead of from an arbitrary
start state to and arbitrary end state.

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@bouthilx
Copy link
Member

bouthilx commented May 9, 2022

Is this PR waiting for a final review from my side? Sorry I lost track in the past 2 weeks.

@lebrice
Copy link
Collaborator Author

lebrice commented May 9, 2022

Is this PR waiting for a final review from my side? Sorry I lost track in the past 2 weeks.

Yep. I'll fix the merge conflict tomorrow, but apart from that, it's ready on my end

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Copy link
Member

@bouthilx bouthilx left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
@lebrice lebrice merged commit 215d781 into Epistimio:develop May 13, 2022
@bouthilx bouthilx added the ci label May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants