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

Experimenter bug #176

Merged
merged 5 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions swmmanywhere/paper/experimenter.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,16 @@ def process_parameters(jobid: int,
"param",
"value"]].itertuples(index=False,
name=None):
if grp not in overrides:

# Experimenter overrides take precedence over the config file
if grp in config.get('parameter_overrides',{}):
overrides[grp] = config['parameter_overrides'][grp]
elif grp not in overrides:
overrides[grp] = {}
overrides[grp][param] = val
config['parameter_overrides'].update(overrides)

overrides[grp][param] = val
config['parameter_overrides'] = overrides

# Run the model
config['model_number'] = ix
logger.info(f"Running swmmanywhere for model {ix}")
Expand Down
38 changes: 37 additions & 1 deletion tests/test_experimenter.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""Tests for the main experimenter."""
from __future__ import annotations

from unittest import mock

import numpy as np

from swmmanywhere import parameters
Expand Down Expand Up @@ -41,4 +43,38 @@ def test_generate_samples():
seed = 1,
groups = True)
assert len(samples) == 36


def test_process_parameters():
"""Test process_parameters."""
config = {'parameters_to_sample' : ['min_v','max_v'],
'sample_magnitude' : 3,
}

# Test standard
with mock.patch('swmmanywhere.paper.experimenter.swmmanywhere.swmmanywhere',
return_value=('fake_path',{'fake_metric' : 1})) as mock_sa:
result = experimenter.process_parameters(0,1,config)

assert len(result[0]) == 48
assert_close(result[0][0]['min_v'], 0.310930)

# Test experimenter takes precedence over overrides
config['parameter_overrides'] = {'hydraulic_design': {'min_v': 1.0}}
with mock.patch('swmmanywhere.paper.experimenter.swmmanywhere.swmmanywhere',
return_value=('fake_path',{'fake_metric' : 1})) as mock_sa:
result = experimenter.process_parameters(0,1,config)

assert len(result[0]) == 48
assert_close(result[0][0]['min_v'], 0.310930)
Comment on lines +61 to +68
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this testing that the experimenter overrides do anything at all? The results you are asserting are identical to the previous case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The point is that the experimenter parameter selection (passed via parameter_overrides) takes precedence over a config defined parameter override. It should be the same as before even though we have provided an override.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For this and subsequent comments - I have tried to address by improving the docstring of process_parameters


# Test non experimenter overrides still work
config['parameter_overrides'] = {'hydraulic_design': {'max_fr': 0.5}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a non-experimenter override?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The experimenter passes parameters to main function via parameter_overrides in the config file. The parameters it is sampling are min_v and max_v. So max_fr is a 'non-experimenter override', i.e., one that is not provided by the experimenter function.

If I am running the experimenter and provide my own overrides that are nothing to do with parameters_to_sample I want to check that they are still set

with mock.patch('swmmanywhere.paper.experimenter.swmmanywhere.swmmanywhere',
return_value=('fake_path',{'fake_metric' : 1})) as mock_sa:
result = experimenter.process_parameters(0,1,config)

for call in mock_sa.mock_calls:
assert call.args[0]['parameter_overrides']['hydraulic_design']['max_fr'] == 0.5

assert len(result[0]) == 48
assert_close(result[0][0]['min_v'], 0.310930)
Comment on lines +79 to +80
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, no change in the assert statements.

Copy link
Collaborator Author

@barneydobson barneydobson May 24, 2024

Choose a reason for hiding this comment

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

The setting of max_fr should not have interacted with the sampling of min_v - this is testing that