Issue #1248 Write svat_groundwater to file and update the tests #1249
Conversation
|
There was a problem hiding this comment.
Nice work! Thanks for adding these tests. I have some suggestions to improve the code and I also see some tests failing for regridding, could you try to fix these?
Also: we need to remind ourselves that this will result in primod requiring updates to its fixtures.
| Changed | ||
| ~~~~~~~ | ||
|
|
||
| - Variable ``max_abstraction_groundwater`` in :class:`imod.msw.Sprinkling` now needs to |
There was a problem hiding this comment.
You forgot max_abstraction_surfacewater, please add that
| def ravel_per_subunit(array: xr.DataArray) -> np.ndarray: | ||
| # per defined well element, all subunits | ||
| array_out = array.to_numpy()[:, well_row, well_column].ravel() |
There was a problem hiding this comment.
Please pass well_row and well_column to this function, like so:
| def ravel_per_subunit(array: xr.DataArray) -> np.ndarray: | |
| # per defined well element, all subunits | |
| array_out = array.to_numpy()[:, well_row, well_column].ravel() | |
| def ravel_per_subunit(array: xr.DataArray, well_row: np.ndarray, well_column: np.ndarray) -> np.ndarray: | |
| # per defined well element, all subunits | |
| array_out = array.to_numpy()[:, well_row, well_column].ravel() |
I'd also prefer it if you put this helper function outside the class and add it above in this module. You can name it _ravel_per_subunit to show it is a semi-private helper function.
| layer_source = ravel_per_subunit( | ||
| layer_per_svat.where(max_rate_per_svat > 0) | ||
| ).astype(dtype=np.int32) | ||
| svat_source_target = ravel_per_subunit( | ||
| svat.where(max_rate_per_svat > 0) | ||
| ).astype(dtype=np.int32) |
There was a problem hiding this comment.
You can reduce code complexity and duplication somewhat like this:
| layer_source = ravel_per_subunit( | |
| layer_per_svat.where(max_rate_per_svat > 0) | |
| ).astype(dtype=np.int32) | |
| svat_source_target = ravel_per_subunit( | |
| svat.where(max_rate_per_svat > 0) | |
| ).astype(dtype=np.int32) | |
| is_active_per_svat = max_rate_per_svat > 0 | |
| layer_source = ravel_per_subunit( | |
| layer_per_svat.where(is_active_per_svat) | |
| ).astype(dtype=np.int32) | |
| svat_source_target = ravel_per_subunit( | |
| svat.where(is_active_per_svat) | |
| ).astype(dtype=np.int32) |
| } | ||
|
|
||
| for var in self._with_subunit: | ||
| array = self.dataset[var].where(max_rate_per_svat > 0).to_numpy() |
There was a problem hiding this comment.
See previous suggestion:
| array = self.dataset[var].where(max_rate_per_svat > 0).to_numpy() | |
| array = self.dataset[var].where(is_active_per_svat).to_numpy() |
Fixes #1248
Description
This PR fixes Issue #1248 by removing the
svat_groundwaterfrom the _to_fill variable. For now the source svat == target svat.In addition the
max_abstraction_groundwatershould now contains the subunit coordinate. The current implementation assigned the abstraction to all stacked subunits. This is not desirable in case the second svat represents the urban area for example. Now its up to the user to define irrigation at the subunit level.Checklist
Issue #nr, e.g.Issue #737