-
Notifications
You must be signed in to change notification settings - Fork 0
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 #344 part1 imod refactor primod update #348
Issue #344 part1 imod refactor primod update #348
Conversation
@@ -29,6 +28,11 @@ scipy = "*" | |||
tomli = "*" | |||
tomli-w = "*" | |||
xmipy = "*" | |||
filelock = "*" |
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.
Could you sort the dependencies?
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 defer from that now, as this is a temporary measure. After iMOD Python has been released with the API update, I will revert these changes to the pixi.toml. Instead, I will then pin iMOD Python to be above or higher a specific version number.
@@ -71,7 +73,7 @@ def _create_well_id( | |||
layer = np.tile(well_layer, (n_subunit, 1)) | |||
layer_1d = layer[well_active] | |||
|
|||
well_id = self.well.dataset.coords["index"] + 1 | |||
well_id = well_cellid.coords["ncellid"] + 1 |
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.
ncellid is actually an zero based index array right? otherwise the +1 is now unnecessary.
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.
That is correct!
@@ -17,7 +17,7 @@ def make_msw_model( | |||
|
|||
idomain = gwf["GWF_1"]["dis"]["idomain"] | |||
if "layer" in idomain.dims: | |||
idomain_layer1 = idomain.sel(layer=1) | |||
idomain_layer1 = idomain.sel(layer=1, drop=True) |
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.
Why did you do this?
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 ran into an issue with some tests, where iMOD Python was complaining about faulty coordinates. It turned out as the layer coord was passed on to imod.util.spatial.get_cell_area
, which throws an error if there is any coord more than x, y, dx, and dy. This could be easily fixed by dropping the layer coordinate upon selecting the first layer in the fixture. It did not seem like the layer coord was necessary in the first place, so I think dropping it is a slight improvement regardless.
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.
Nice work, I had a few comments. Also the testbench is red for primod and the coupler itself.
…using the dtype of like, which is an integer. This causes problem for the fill value "nan" which the ribamod coupling depends on.
bbc941e
into
issue_#342_imod_refactor_primod_update
UPDATE: This is a new attempt for a PR, in the other one I accidentily merged locally and pushed to feature branch. I reset that now.
Closes #344, this is the first task to update
primod
for the iMOD Python refactor.Plans are layed out in #342. In this first task, I changed the use of the deprecated
WellDisStructured
object, to the Mf6Wel object, which is a direct translation of MODFLOW 6's cellids to a file. As this iMOD Python refactor is still work in progress, I temporarily introduce a install-imod task, which pip installs imod from a git branch, instead of a release on conda-forge. After I have accomplished part 2 of the refactors, I'll make an iMOD Python release, and this can be reverted back.It does the following things:
Mf6Wel
object instead of theWellDisStructured
objectdtype
in calls to imod.prepare.rasterize to float, to work around this issue Primod's basin mapping can't deal with integer basin id's with latest Rasterio dependecy #333. Thedtype
argument is only included in the the dev version of iMOD Python yet.