Closed davidorme closed 2 months ago
@vgro With the last commit, all of the Hydrology tests now pass. You can see the specific alterations here (since your changes and excluding the merge up of the base branch #421):
Main things:
The test of which variables are returned in test_setup_hydrology_input_current_timestep
was not working as expected. I've updated the test so it checks the returned and expected variable names match, but had to edit the expected list. Are those correct?
In hydrology_tools.setup_hydrology_input_current_timestep
, the new code was returning 2D arrays for the surface layers, which was inconsistent with some of the other surface layer variables. The example I found was in the section below where the two variables being averaged had different dimensions - the river_discharge_rate
entries were (n_cells,)
but the aerodynamic_resistance_surface
entries came via wind_speed
and the surface variables and were (1, n_cells)
. I've now squeezed these variables so they are (n_cells,)
.
HydrologyModel.soil_layer_thickness_mm
to be (n_soil_layers, n_cells)
- I think all of the use cases needed it to be at least rotated to a vertical array and it seemed sensible just to make it explicitly 2D and broadcast to cells. One outcome of that is that I've simplified the calculation of soil_moisture
to only handle float values for init_soil_moisture
- at present the model __init__
can only take a float value here from config, so that seemed sane. We may want to make initial_soil_moisture
a data variable at some point, and then things change.~The the test values in test_hydrology_model.py::test_setup
have changed. I've substituted the reported values into the tests to check the testing mechanics all work but don't have insight into why these values have changed.~
~I've kept the old values as comments from comparison, if you can review these values and see where I've wrecked it, that would be great. Then we're done with this branch, I think!~
The 'problem' is that the fixture_core_components.layer_structure.soil_layers
is [-0.25, -1. ]
is inconsistent with the depths set in dummy_climate_data
at [-0.5, -1.0]
. If I match those up, it works with the original values with minor tweaks for the new 4th cell.
@vgro With the last commit, all of the Hydrology tests now pass. You can see the specific alterations here (since your changes and excluding the merge up of the base branch #421):
0abf176...444-fixing-hydrology-for-421
Main things:
* The test of which variables are returned in `test_setup_hydrology_input_current_timestep` was not working as expected. I've updated the test so it checks the returned and expected variable names match, but had to edit the expected list. Are those correct? * In `hydrology_tools.setup_hydrology_input_current_timestep`, the new code was returning 2D arrays for the surface layers, which was inconsistent with some of the other surface layer variables. The example I found was in the section below where the two variables being averaged had different dimensions - the `river_discharge_rate` entries were `(n_cells,)` but the `aerodynamic_resistance_surface` entries came via `wind_speed` and the surface variables and were `(1, n_cells)`. I've now squeezed these variables so they are `(n_cells,)`.
* I've explicitly reshaped `HydrologyModel.soil_layer_thickness_mm` to be `(n_soil_layers, n_cells)` - I think all of the use cases needed it to be at least rotated to a vertical array and it seemed sensible just to make it explicitly 2D and broadcast to cells. One outcome of that is that I've simplified the calculation of `soil_moisture` to only handle float values for `init_soil_moisture` - at present the model `__init__` can only take a float value here from config, so that seemed sane. We may want to make `initial_soil_moisture` a data variable at some point, and then things change.
~Problems!~
~The the test values in
test_hydrology_model.py::test_setup
have changed. I've substituted the reported values into the tests to check the testing mechanics all work but don't have insight into why these values have changed.~~I've kept the old values as comments from comparison, if you can review these values and see where I've wrecked it, that would be great. Then we're done with this branch, I think!~
The 'problem' is that the
fixture_core_components.layer_structure.soil_layers
is[-0.25, -1. ]
is inconsistent with the depths set indummy_climate_data
at[-0.5, -1.0]
. If I match those up, it works with the original values with minor tweaks for the new 4th cell.
LGTM, few minor points, then you can merge it:
the variable names are not all correct, it should be soil_moisture
not soil_moisture_mm
, see inline comment. The layer related variables can be deleted.
The 2D arrays for surface values are meant to be 1D and I'm happy with the squeeze option if that is the best way to deal with it.
The idea behind arrays for initial_soil_moisture
was that different grid cells might have different initial values, for example if we put in an external dataset. But this is not happening anytime soon, and like you said the it's not setup, so limiting this to float is fine.
New returned values in test_hydrology_model.py::test_setup
are correct, you can delete the values in comments.
Jeepers - had the base for the merge set to develop
and nearly bypassed protections to merge it into that.
Right. Anyway, all the tests for the hydrology
model now pass locally, so this branch has done its job of fixing this up for #421 and I'll merge it down.
This is a WIP PR to adapt the hydrology model and tests to use the revised LayerStructure.
@vgro - I haven't merged in #441 yet, so this was pushed without verify and there are a bunch of
mypy
complaints. My thinking here was that I wanted you to see just the changes I've made to get a feel for them, before they get a bit hidden when the LayerStructure implementation changes come in.When I run these changes locally with the changes in #441, nearly all the test pass, but
test_hydrology_model/test_setup
fails and so doestest_hydrology_tools.py::test_setup_hydrology_input_current_timestep
Fixes #444 (issue)
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks