Experimenter bug #176
Experimenter bug #176
Conversation
fix parameter setting bug
| 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) |
There was a problem hiding this comment.
How is this testing that the experimenter overrides do anything at all? The results you are asserting are identical to the previous case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}} |
There was a problem hiding this comment.
Why is this a non-experimenter override?
There was a problem hiding this comment.
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
| assert_close(result[0][0]['min_v'], 0.310930) |
There was a problem hiding this comment.
Again, no change in the assert statements.
There was a problem hiding this comment.
The setting of max_fr should not have interacted with the sampling of min_v - this is testing that
Description
There was a bug in
process_parametersto do with parameter setting. Now fixed and tests added so that the case is properly avoided.