Issue #365 add regrid to msw packages #982
Conversation
There was a problem hiding this comment.
Looks good so far, I'm quite impressed adding regridding support doesn't seem to require that much additions to the existing code base. Fruits of our labour generalizing things, and moving logic outside Modflow6 package objects.
I miss regridding of the most crucial grid package though: GridData. The landuse variable here can be regridded with the mode (or modus?) method.
Furthermore you changed some things to the infiltration package input arguments without explanation; so I'd like to know the reasoning behind that.
Grid packages that still require regridding:
GridData(landusevariable can usemodeoption``)sprinkling(Still requires some work for the grid agnostic well, that might be the reason you haven't done it yet here)IdfMapping
| "downward_resistance": (RegridderType.OVERLAP, "mean"), | ||
| "upward_resistance": ( | ||
| RegridderType.OVERLAP, | ||
| "mean", | ||
| ), |
There was a problem hiding this comment.
These are vertical resistances, meaning thickness/conductivity. Vertical hydraulic conductivity is regridded with the harmonic_mean method. Shouldn't we use this one as well here? Perhaps @HendrikKok has an opinion on this.
There was a problem hiding this comment.
I have introduced the same method for all parameters, and left the fine-tuning of choosing the correct parameter for each array for a future issue. If these are permeability-like parameters then it indeed be harmonic as it deals with parallel resistors. But since I couldn't find how these inputs are used by metaswap,, I wouldn't know how to upscale them. Besides @HendrikKok , also this "Paul" person could be consulted for this I guess.
There was a problem hiding this comment.
Could you open an issue that we still need to select appropriate methods?
| RegridderType.OVERLAP, | ||
| "mean", | ||
| ), | ||
| "bottom_resistance": (RegridderType.OVERLAP, "mean"), |
There was a problem hiding this comment.
See other comment about regridding resistances
| if interception_capacity_per_LAI is not None: | ||
| self.dataset["interception_capacity_per_LAI_Rutter"] = ( | ||
| interception_capacity_per_LAI | ||
| ) | ||
| else: | ||
| self.dataset["interception_capacity_per_LAI_Rutter"] = ( | ||
| interception_capacity_per_LAI_Rutter | ||
| ) | ||
| if interception_capacity_per_LAI is not None: | ||
| self.dataset["interception_capacity_per_LAI_VonHoyningen"] = ( | ||
| interception_capacity_per_LAI | ||
| ) | ||
| else: | ||
| self.dataset["interception_capacity_per_LAI_VonHoyningen"] = ( | ||
| interception_capacity_per_LAI_VonHoyningen | ||
| ) |
There was a problem hiding this comment.
What were your reasons to change this? This adds more complexity to the code. I also see no changes to the docstring.
There was a problem hiding this comment.
The reason is this:
-in most datasets, the list of keys in the dataset matches the arguments of the__init__ function of the package
-so in _regrid we can regrid all the arrays, and then call __init__ on the regridded package with the regridded arrays
-but for this package, the arguments of the __init__ function did not match the keys of the package dataset because one init parameter( xxx) was placed in the dataset under 2 names: xxx_VonHoyningen and xxx_Rutter
-so now I added the option to pass xxx_VonHoyningen and xxx_Rutter directly to the initializer , so the _regrid function will work just like for any other package.
-I also preserved the original parameter so that user code wil keep working
-I now updated the docstring to inform users that the new init-parameters are for internal use and the user should ignore them
There was a problem hiding this comment.
Thanks for the explanation, this makes sense.
|
luitjansl
merged commit 0038faf
into
metaswap_regridding_feature_branch
Fixes #365
Description
Adds regrid functionality to a number of metaswap packages
Adds unittests for regridding these packages
Checklist
Issue #nr, e.g.Issue #737