Issue #728 metaswap grid agnostic well #1256
Conversation
imod/msw/sprinkling.py
Outdated
| well_row = self.well["row"] - 1 | ||
| well_column = self.well["column"] - 1 | ||
| well_layer = self.well["layer"] | ||
| cellid = self.well["cellid"] |
There was a problem hiding this comment.
You could rename this to well_cellid
There was a problem hiding this comment.
Done for this local variable, also in the CouplerMapping equivalent function
imod/msw/sprinkling.py
Outdated
| if len(cellid.coords["nmax_cellid"]) != 3: | ||
| raise TypeError("Coupling to unstructured grids is not supported.") | ||
|
|
||
| well_layer = cellid.sel(nmax_cellid="layer").data |
There was a problem hiding this comment.
What is nmac_cellid? I cant figure it our by just looking at this file
Could you rename it to something more describing?
There was a problem hiding this comment.
I based this name on an example I found in the UGRID conventions. There it is named nMaxMesh2_face_nodes, meaning max amount of nodes per face. The "max" is relevant in Ugrids, as meshes can be flexible so the amount of nodes per face can vary in the grid.
In our case this is not happening, so the "max" is wrong here. I'll rename to ncellid, as that is a better name for this dimension.
There was a problem hiding this comment.
Looking better at this: ncellid already refers to the rows in the table. nmax_cellid refers to columns, maybe ndim_cellid is a better name?
There was a problem hiding this comment.
After a brief discussion with @Manangka, I renamed it to dim_cellid.
imod/mf6/wel.py
Outdated
| @@ -248,7 +320,7 @@ def _create_cellid( | |||
| df_for_cellid = assigned_wells.groupby("index").first() | |||
There was a problem hiding this comment.
Would unique_assigned_wells be a better name?
There was a problem hiding this comment.
Indeed, renamed
| @@ -248,7 +320,7 @@ def _create_cellid( | |||
| df_for_cellid = assigned_wells.groupby("index").first() | |||
| d_for_cellid = df_for_cellid[["x", "y", "layer"]].to_dict("list") | |||
|
|
|||
| return self._derive_cellid_from_points(like, **d_for_cellid) | |||
| return derive_cellid_from_points(like, **d_for_cellid) | |||
There was a problem hiding this comment.
If the well is ever moved the cellid needs to be updated.
Thus method then needs to be called whenever the row,column or layer is adjusted
I don't know how likely that is but i cant imagine a use cage where you start with a base model with wells and then move some of them to see the effect of it.
There was a problem hiding this comment.
This is not a likely use case, as the Mf6Wel is always generated at writing, so after regridding/clipping etc.
imod/mf6/wel.py
Outdated
| @@ -248,7 +320,7 @@ def _create_cellid( | |||
| df_for_cellid = assigned_wells.groupby("index").first() | |||
| d_for_cellid = df_for_cellid[["x", "y", "layer"]].to_dict("list") | |||
There was a problem hiding this comment.
I find the name cellid confusing. In MODFLOW terms this is the row, column, layer or the 2dcellid, layer.
In this case it would call it something like id, identifier, name or well_id
There was a problem hiding this comment.
But row, column, layer is what I refer to with the cellid. At some point in the future we might also have to support 2dcellid, but that will probably not happen soon.
|
JoerivanEngelen
merged commit cb9337d
into
issue_#1255_msw_refactoring
Fixes #728
Description
This PR updates two MetaSWAP packages that depend on wells to use of the deprecated
WellDisStructuredobject to theMf6Welobject.It fixes the first small step of the big metaswap refactor. I chose a feature branch as base branch to merge into, to still have the option to create some hotfixes for a new release of iMOD Python while primod is being updated.
I also renamed some variable names and dimension names with clearer names. Furthermore, I type annotated some method.
Checklist
Issue #nr, e.g.Issue #737