Issue #1255 msw refactor mf6 obj at write #1259
Conversation
JoerivanEngelen
changed the base branch from
master
to
msw_refactoring_feature_branch
imod/msw/coupler_mapping.py
Outdated
|
|
||
| def _render(self, file, index, svat: xr.DataArray): | ||
| self._create_mod_id_rch(svat) | ||
| # package check only possible after calling _create_mod_id_rch |
There was a problem hiding this comment.
Is this comment still true? Before _create_mod_id_rch changes something in self.dataset. After your changes it doesn't anymore. How does calling this method affect the outcome of self._pkgcheck?
There was a problem hiding this comment.
Good catch, this is indeed not relevant anymore. Removed confusing comment.
| if mf6_well: | ||
| well_data_dict = self._create_well_id(svat, idomain_active, mf6_well) | ||
| for key in data_dict.keys(): | ||
| data_dict[key] = np.append(well_data_dict[key], data_dict[key]) |
There was a problem hiding this comment.
Don;t know if this is better but below an alternative way of writting this
appended_well_data = {key: np.append(well_data_dict[key], data_dict[key]) for key in data_dict.keys()}
data_dict.update(appended_well_data)
There was a problem hiding this comment.
That is also a good way of doing things. It has the slight advantage that there is an additional variable being named, which might improve reality a bit, though updating keys in a for loop is such a basic thing in python that most developers will not struggle with this part of the code.
The slight advantage of the present approach is a slightly reduced memory use, as variables are updated one by one, instead of creating a full dictionary appended_well_data first and updating the existing dict with it. Though I do not think in most cases memory use here will matter a lot.
I think I'll stick to the present approach, it is a bit of a micro-optimalization; I do not have a strong opinion on the best approach, and I don't think it will matter a lot in the end.
| self, | ||
| directory: Union[str, Path], | ||
| mf6_dis: StructuredDiscretization, | ||
| mf6_wel: Mf6Wel, |
There was a problem hiding this comment.
Does this mean there can only be one well in the model?
There was a problem hiding this comment.
Only one MODFLOW6 well package can be coupled to MetaSWAP. The MODFLOW6 groundwater model itsself can have more well packages.
| def __init__( | ||
| self, | ||
| max_abstraction_groundwater: xr.DataArray, | ||
| max_abstraction_surfacewater: xr.DataArray, | ||
| well: Mf6Wel, |
There was a problem hiding this comment.
I cant find in the code base where this object is instantiated except for some test classes. So i don't know how feasible the comment below is
Would it be possible to introduce a view or wrapper around the original Well object?
When the well object is replaced outside this class the view/wrapper needs to be updated
The advantage is that you can pass the wrapper to the init method which avoids extending the write and render with arguments that other packages don't need but are now forced to accept
@dataclass
class Mf6WelView
well : Mf6Wel
There was a problem hiding this comment.
The MODFLOW6 simulation also only generates Mf6Wel objects during the call to write.
The disadvantage of giving a Mf6Wel to the init is that it complicates the Sprinkling.regrid_like operation, as extra logic has to be introduced to regrid Mf6Wel objects.
Furthermore the Sprinkling.dump method, which we want to implement, would require extra logic to separate the Well attribute from the Sprinkling class, separately store the well in a NetCDF, and load the Well NetCDF again and attaching it to Sprinkling.well in the Sprinkling.from_file constructor.
|
JoerivanEngelen
merged commit 171ee44
into
msw_refactoring_feature_branch
Fixes #1255 and #1056
Description
This is part 2 of the metaswap refactoring on the iMOD Python side. The goal of this step was to require a Mf6Wel object at
writeinstead of at init. For this I was struggling quite a lot with Linkov's principle and the resulting mypy errors. In the end, I resorted to requiring all arguments for the write method for each package. This results in a bit of a weird situation, where aNonehas to be provided for themf6_disandmf6_welarguments to thewriteof an individual no even using these args. I don't think this is a major hickup for users, as they are nearly always writing withmodel.writedirectly, and not writing individual packages manually.Though not perfect, I think this is certainly a big improvement to the current situation.
In total, this PR:
SprinklingandCouplerMapping. This makes dumping the metaswap packages easier. This affects the filesimod.msw.sprinkling.pyandimod.msw.coupler_mapping.py.SprinklingandCouplerMappingclass to ask MODFLOW6 packages atwriteinstead of at__init__. This affectsimod.msw.sprinkling.py,imod.msw.coupler_mapping.py, andimod.msw.model.pyregrid_likefor the Sprinkling package. Affectsimod.msw.sprinkling.py.Mf6Welpackage has to be created manually now after regridding.Checklist
Issue #nr, e.g.Issue #737