-
Notifications
You must be signed in to change notification settings - Fork 52
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
Configurator Unit Tests #174
Configurator Unit Tests #174
Conversation
Added __init__.py for test module. Added test_configurators.py. Added basic fixtures and construction tests.
Added remaining unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! I think this is a solid basis, but we need to be a little bit more thorough with the test parametrization.
|
||
@pytest.fixture() | ||
def test_shape(): | ||
return (5,7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8: space after comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixture should also be parametrized: We want to check for multiple different batch/set sizes and data dims. Ideally, these are minimally small, e.g.:
@pytest.fixture(params=[2, 3])
def batch_size(request):
return request.param
and same for set_size
and num_features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to conftest.py
and parameterized over [2,3]
(see b2922ca)
|
||
|
||
@pytest.fixture() | ||
def random_data_no_output(test_shape): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't split fixtures like this. Instead, use parametrization:
@pytest.fixture(params=[True, False])
def random_data(request):
data = ...
if request.param:
data["summary_outputs"] = ...
return data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combined repetitive fixtures in conftest.py
(see b2922ca)
|
||
|
||
@pytest.fixture() | ||
def test_params(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also parametrize this fixture. We should have runs where zero/one/multiple variables are used for each input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameterized over [True, False]
(see b2922ca)
assert config.inference_variables == test_params['inference_variables'] | ||
assert config.inference_conditions == [] | ||
assert config.summary_variables == [] | ||
assert config.summary_conditions == [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test removed (see b2922ca)
'var2': keras.random.normal(test_shape), | ||
'var3': keras.random.normal(test_shape), | ||
'summary_outputs': keras.random.normal(test_shape), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add summary inputs / conditions to this fixture, which should have shape (batch_size, set_size, ...). Note that images should be able to be handled by the configurator as both regular and set input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added "summary_inputs"
and "summary_conditions"
to the random_data
fixture in conftest.py
(see b2922ca). Is this what you were looking for?
def test_inference_vars_filter(random_data, configurator: Configurator, test_shape): | ||
config = configurator | ||
filtered_data = config.configure_inference_variables(random_data) | ||
assert filtered_data.shape == test_shape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test only works if there is only a single inference_variable, i.e. no concatenation actually happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored test to account for multiple tensors (see b2922ca)
filtered_inference_conds = config.configure_inference_conditions(random_data_no_output) | ||
filtered_summary_vars = config.configure_summary_variables(random_data_no_output) | ||
filtered_summary_conds = config.configure_summary_conditions(random_data_no_output) | ||
assert filtered_inference_conds == None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use is None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instances of ... == None
replaced with ... is None
(see b2922ca)
|
||
# Test for correct construction of Configurator with all args | ||
def test_configurator_init(test_params, configurator: Configurator): | ||
config = configurator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This renaming has some repeat code smell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed config = configurator
instances and used the respective function arg instead (see b2922ca)
Separated fixtures and placed them in conftest.py
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## streamlined-backend #174 +/- ##
======================================================
Coverage ? 15.24%
======================================================
Files ? 107
Lines ? 5181
Branches ? 0
======================================================
Hits ? 790
Misses ? 4391
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Added batch_size, set_size, and num_features parameterizations in conftest.py. Combined repetitive fixtures in conftest.py. Combined repetitive tests in test_configurators.py. Parameterized Configurator initialization in conftest.py. Parameterized parameter selection in conftest.py. Removed initialization tests in test_configurators.py. Added summary_inputs and summary_conditions to parameters. Changed instances of '==None' to 'is None'. Removed 'config=Configurator' instances in test_configurators.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for the PR
commit 320f1ae Author: lars <[email protected]> Date: Tue Jun 18 16:23:01 2024 +0200 fix two moons simulated dtype commit 27f99cd Author: lars <[email protected]> Date: Tue Jun 18 16:09:45 2024 +0200 fix data modification for tensorflow compiled mode commit c8060fc Merge: 3150d11 e2355de Author: lars <[email protected]> Date: Tue Jun 18 15:35:59 2024 +0200 Merge remote-tracking branch 'origin/streamlined-backend' into streamlined-backend commit 3150d11 Author: lars <[email protected]> Date: Tue Jun 18 15:35:52 2024 +0200 add JAX Approximator finalize all Approximators commit e2355de Author: Chase Grajeda <[email protected]> Date: Tue Jun 18 22:15:37 2024 +0900 Configurator Unit Tests (bayesflow-org#174) * First additions Added __init__.py for test module. Added test_configurators.py. Added basic fixtures and construction tests. * Remaining tests Added remaining unit tests * Added conftest Separated fixtures and placed them in conftest.py * Added requested changes Added batch_size, set_size, and num_features parameterizations in conftest.py. Combined repetitive fixtures in conftest.py. Combined repetitive tests in test_configurators.py. Parameterized Configurator initialization in conftest.py. Parameterized parameter selection in conftest.py. Removed initialization tests in test_configurators.py. Added summary_inputs and summary_conditions to parameters. Changed instances of '==None' to 'is None'. Removed 'config=Configurator' instances in test_configurators.py.
* Squashed commit of the following: commit 320f1ae Author: lars <[email protected]> Date: Tue Jun 18 16:23:01 2024 +0200 fix two moons simulated dtype commit 27f99cd Author: lars <[email protected]> Date: Tue Jun 18 16:09:45 2024 +0200 fix data modification for tensorflow compiled mode commit c8060fc Merge: 3150d11 e2355de Author: lars <[email protected]> Date: Tue Jun 18 15:35:59 2024 +0200 Merge remote-tracking branch 'origin/streamlined-backend' into streamlined-backend commit 3150d11 Author: lars <[email protected]> Date: Tue Jun 18 15:35:52 2024 +0200 add JAX Approximator finalize all Approximators commit e2355de Author: Chase Grajeda <[email protected]> Date: Tue Jun 18 22:15:37 2024 +0900 Configurator Unit Tests (#174) * First additions Added __init__.py for test module. Added test_configurators.py. Added basic fixtures and construction tests. * Remaining tests Added remaining unit tests * Added conftest Separated fixtures and placed them in conftest.py * Added requested changes Added batch_size, set_size, and num_features parameterizations in conftest.py. Combined repetitive fixtures in conftest.py. Combined repetitive tests in test_configurators.py. Parameterized Configurator initialization in conftest.py. Parameterized parameter selection in conftest.py. Removed initialization tests in test_configurators.py. Added summary_inputs and summary_conditions to parameters. Changed instances of '==None' to 'is None'. Removed 'config=Configurator' instances in test_configurators.py. * Added loss test Added test for post-training loss < pre-training loss to test_fit.py::test_fit * Added vanishing weights test Added test in test_fit.py::test_fit for vanishing weights * Added simulator test Added test to test_fit.py for verifying the simulator produces random and consistent data * Added MMD test Added MMD test to test_two_moons.py. Added MMD method to utils/ops.py. Added test_dataset to test_two_moons/conftest.py. * Linting adjustments Added auto-formatting changes from ruff
Added basic unit tests to the
Configurator
experimental class, focusing on initialization and correct shape output from the respective configuration filter methods.The following tests are currently implemented:
summary_outputs
summary_outputs
is present indata
summary_outputs
is present indata
andkeys
keys
Feel free to recommend more tests for the suite!