Skip to content
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 #965 npf from imod5 #1010

Merged
merged 16 commits into from
May 7, 2024

Conversation

luitjansl
Copy link
Contributor

@luitjansl luitjansl commented May 6, 2024

Fixes #965

Description

Adds a function to import an npf package from an imod5 project file. Adds tests.

Checklist

  • Links to correct issue
  • Update changelog, if changes affect users
  • PR title starts with Issue #nr, e.g. Issue #737
  • Unit tests were added
  • If feature added: Added/extended example

Copy link
Contributor

@JoerivanEngelen JoerivanEngelen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I have some suggestions to improve things

imod/mf6/npf.py Outdated
Comment on lines 544 to 551
def fill_missing_layers(
source: xr.DataArray, full: xr.DataArray, fillvalue: Union[float | int]
):
result = zeros_like(full)
result.values[:, :, :] = fillvalue
for l in source.coords["layer"].values:
result.loc[l, :, :] = source.sel({"layer": l})
return result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if regridder_types is not None:
regridder_settings.update(regridder_types)

regrid_context = RegridderWeightsCache(data["k"], target_grid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: I think we should be able to optionally provide regrid contexts when we implement complete conversion of models.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can just provide an optional parameter there which has a default value of an empty RegridderCache. Then it's up to the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that is a good resolution. You could implement immediately so we don't forget. If you decide to do so, please don't forget to update the method for the discretization package as well.

luitjansl and others added 5 commits May 6, 2024 17:45
review comment

Co-authored-by: Joeri van Engelen <[email protected]>
nice!

Co-authored-by: Joeri van Engelen <[email protected]>
…es/imod-python into issue_#965_npf_from_imod5

# Conflicts:
#	imod/mf6/npf.py
source for the layers that are in the source. For the other layers, the
fillvalue is assigned.
"""
result = zeros_like(full)
Copy link
Contributor

@Huite Huite May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty confident xarray has tooling for this, which ideally also supports out-of-memory (unlike accessing values).

Maybe align?

import xarray as xr
da1 = xr.DataArray([1, 2, 4], coords={"x": [1, 2, 4]}, dims=["x"])
da2 = xr.DataArray([0, 0, 0, 0, 0], coords={"x": [1, 2, 3, 4, 5]}, dims=["x"])
aligned, _ = xr.align(da1, da2, join="right")

https://docs.xarray.dev/en/stable/generated/[xarray.align](https://docs.xarray.dev/en/stable/generated/xarray.align.html).html

Copy link
Contributor

@JoerivanEngelen JoerivanEngelen May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a previous comment of me as well. I had a code suggestion using reindex, but it appears to be ignored or not pushed yet?

UPDATE: Found the comment: #1010 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoerivanEngelen Indeed, I overlooked it, Now it is implemented, with reindexing

@luitjansl luitjansl merged commit 721d5c3 into imod5_converter_feature_branch May 7, 2024
7 checks passed
@luitjansl luitjansl deleted the issue_#965_npf_from_imod5 branch May 7, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants