-
Notifications
You must be signed in to change notification settings - Fork 3
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
Issue #969 imod5 import recharge #1031
Issue #969 imod5 import recharge #1031
Conversation
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.
I have some comments!
review comment Co-authored-by: Joeri van Engelen <[email protected]>
review comment Co-authored-by: Joeri van Engelen <[email protected]>
…/Deltares/imod-python into issue_#969_imod5_import_recharge
imod/mf6/rch.py
Outdated
# create an array indicating in which cells rch is active | ||
is_rch_cell = allocate_rch_cells( | ||
ALLOCATION_OPTION.at_first_active, | ||
target_grid, |
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.
The active
grid (idomain == 1
) should be inserted here. This comes from the regridded discretization package. So you therefore require an extra argument to the from_imod5_data
method, namely the regridded discretization package.
For an example of the API of the allocate_<pkg>_cells
, see: https://deltares.github.io/imod-python/user-guide/09-topsystem.html#allocate-river-cells
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.
done
imod/tests/test_mf6/test_mf6_rch.py
Outdated
@pytest.mark.usefixtures("imod5_dataset") | ||
def test_planar_rch_from_imod5_constant(imod5_dataset, tmp_path): | ||
data = deepcopy(imod5_dataset) | ||
target_grid = data["khv"]["kh"] |
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 do not test if the regridding works now... I don't think we need to that for each package, but having one where it is tested helps I think. Could you add a test where you regrid this case to a very coarse grid?
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.
in the latest version of this changeset we use the discretization package as the target grid, and it is much finer than the target_grid = data["khv"]["kh"] that was used earlier. So now it is regridding.
That aside, xugrid does as far as I know the same operation, whether the target grid matches the input grid exactly or not. So it goes through the same code path no matter what target grid we provide.
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.
Some minor comments, approving in advance
imod/mf6/rch.py
Outdated
def from_imod5_data( | ||
cls, | ||
imod5_data: dict[str, dict[str, GridDataArray]], | ||
discretization_package: StructuredDiscretization, |
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.
I think this can be shortened to:
discretization_package: StructuredDiscretization, | |
dis_pkg: StructuredDiscretization, |
As DIS
is an abbreviation used by MODFLOW6 and pkg
is a very common abbreviation for "package".
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.
done
imod/mf6/rch.py
Outdated
imod5_data: dict | ||
Dictionary with iMOD5 data. This can be constructed from the | ||
:func:`imod.formats.prj.open_projectfile_data` method. | ||
target_grid: GridDataArray | ||
The grid that should be used for the new package. Does not | ||
need to be identical to one of the input grids. | ||
regridder_types: dict, optional | ||
Optional dictionary with regridder types for a specific variable. | ||
Use this to override default regridding methods. |
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.
Please update docstring with extra dis_pkg
/discretization_package
argument.
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.
done
data = data[0] | ||
# Fix data for ibound as it contains floating values like 0.34, 0.25 etc. | ||
ibound = data["bnd"]["ibound"] | ||
ibound = ibound.where(ibound <= 0, 1) | ||
data["bnd"]["ibound"] = ibound | ||
_load_imod5_data_in_memory(data) | ||
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.
This cleanup also occurs in the tests test_mf6_dis.py
, please remove the ibound cleanup from these tests.
Also: You might get some increased performance by first loading the data into memory and then cleaning up ibound.
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.
done
review comment Co-authored-by: Joeri van Engelen <[email protected]>
Fixes #969
Description
implements importing recharge packages from a project file. The recharge can be imported using a planar grid,
in which case it assigns the recharge to the uppermost active cell for each column.
The recharge can also be imported from a non-planar (so fully 3d) grid in which case it is just regridded to the target grid.
Checklist
Issue #nr
, e.g.Issue #737