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

Disentangle Modflow objects as much from the MetaSWAP objects as possible #1255

Closed
JoerivanEngelen opened this issue Oct 24, 2024 · 1 comment · Fixed by #1259
Closed

Disentangle Modflow objects as much from the MetaSWAP objects as possible #1255

JoerivanEngelen opened this issue Oct 24, 2024 · 1 comment · Fixed by #1259
Assignees

Comments

@JoerivanEngelen
Copy link
Contributor

JoerivanEngelen commented Oct 24, 2024

At present we are in a state where the MetaSWAP objects contain Modflow objects. I decided to do this, just for conceptual reasons: The relevant modflow well data is contained in the metaswap sprinkling object. I didn't think this trhough enough, as this creates all kinds of headaches: it means these packages also have to be regridded, dumped, clipped in some way. Also if we want to regrid modflow packages from MetaSWAP objects, this means grid agnostic wells have to be assigned, meaning wells have to be assigned to layer, row, column twice: Once for Modflow, once for MetaSWAP.

It is a lot easier to only ask for Modflow data when it's really needed: Upon calling .write.

See my earlier comment:

Doing some more thinking on this: It is probably better to refactor the MetaSWAP code base in such a way, that the MODFLOW objects are only necessary upon writing, instead of already upon initialization. For example for the Sprinkling package:

Current state:

class Sprinkling:
    def __init__(
        self,
        max_abstraction_groundwater: xr.DataArray,
        max_abstraction_surfacewater: xr.DataArray,
        modflow_wel: WellDisStructured
     ):
        self.well = modflow_wel
...

    def write(
        self,
        directory: Union[str, Path],
        index: np.ndarray,
        svat: xr.DataArray,
    ):
...

Initial idea:

class Sprinkling:
    def __init__(
        self,
        max_abstraction_groundwater: xr.DataArray,
        max_abstraction_surfacewater: xr.DataArray,
        modflow_wel: Well | LayeredWell
     ):
        self.well = modflow_wel
...

    def write(
        self,
        directory: Union[str, Path],
        index: np.ndarray,
        svat: xr.DataArray,
        modflow_dis: mf6.StructuredDiscretication
        modflow_npf: mf6.NodePropertyFlow
    ):
...

Proposed:

class Sprinkling:
    def __init__(
        self,
        max_abstraction_groundwater: xr.DataArray,
        max_abstraction_surfacewater: xr.DataArray,
        mf6_wellname: str,
     ):
        self.mf6_wellname = mf6_wellname
...

    def write(
        self,
        directory: Union[str, Path],
        index: np.ndarray,
        svat: xr.DataArray,
        modflow_wel: Mf6Wel
    ):
...

This propsed approach has the following advantages over the initial idea:

  1. Modflow objects are only used when really necessary. This has the advantage that we can directly use the more low-level Mf6Wel object instead of the grid agnostic Well and LayeredWell packages, as the latter still have to be assigned to cells. We avoid having to conduct this twice.
  2. Mutations of Modflow data, for example regrid_like, do not have to be done twice. In the initial idea, regridding would have to be called once for the MetaSWAP model and once for the Modflow6Simulation, as we do not check wether the Modflow package assigned to the two different models is a copy or the same package for each model. The proposed approach also means no calls to Modflow6Package.regrid_like are done outside the mf6 module, reducing clutter a bit.

The same approach can be taken for the CouplerMapping object in iMOD Python, and the NodeSvatMapping, RechargeSvatMapping, WellSvatMapping objects in primod.

The MetaMod.write method would be the place fetch the Modflow packages and pass them on through to MetaSwapModel.write

Originally posted by @JoerivanEngelen in #728

@github-project-automation github-project-automation bot moved this to 📯 New in iMOD Suite Oct 24, 2024
@JoerivanEngelen JoerivanEngelen moved this from 📯 New to 📝Refined in iMOD Suite Oct 24, 2024
@JoerivanEngelen
Copy link
Contributor Author

JoerivanEngelen commented Oct 24, 2024

Just a somewhat related frustration: Right now 5 files are required for coupling Modflow models to MetaSWAP: 2 for MetaSWAP, 3 for iMOD Coupler. In an ideal world, the 2 files for MetaSWAP wouldn't be necessary, and the software would receive its cellids to couple to directly from iMOD Coupler.

If this would be the case, the refactor would only affect primod.

@JoerivanEngelen JoerivanEngelen self-assigned this Oct 24, 2024
@JoerivanEngelen JoerivanEngelen moved this from 📝Refined to 🏗 In Progress in iMOD Suite Oct 30, 2024
@JoerivanEngelen JoerivanEngelen moved this from 🏗 In Progress to 🧐 In Review in iMOD Suite Oct 30, 2024
JoerivanEngelen added a commit that referenced this issue Nov 1, 2024
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 ``write`` instead 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 a ``None`` has to be provided for the
``mf6_dis`` and ``mf6_wel`` arguments to the ``write`` of an individual
no even using these args. I don't think this is a major hickup for
users, as they are nearly always writing with ``model.write`` directly,
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:

- Removes the mf6 packages as attributes from ``Sprinkling`` and
``CouplerMapping``. This makes dumping the metaswap packages easier.
This affects the files ``imod.msw.sprinkling.py`` and
``imod.msw.coupler_mapping.py``.
- Updates the ``Sprinkling`` and ``CouplerMapping`` class to ask
MODFLOW6 packages at ``write`` instead of at ``__init__``. This affects
``imod.msw.sprinkling.py``, ``imod.msw.coupler_mapping.py``, and
``imod.msw.model.py``
- Updates the signatures
- Implements a ``regrid_like`` for the Sprinkling package. Affects
``imod.msw.sprinkling.py``.
- Adds a test to regrid a MetaSWAP model and its coupled MODFLOW6 model.
The ``Mf6Wel`` package has to be created manually now after regridding.

# Checklist
- [x] Links to correct issue
- [x] Update changelog, if changes affect users
- [x] PR title starts with ``Issue #nr``, e.g. ``Issue #737``
- [x] Unit tests were added
- [ ] **If feature added**: Added/extended example
@github-project-automation github-project-automation bot moved this from 🧐 In Review to ✅ Done in iMOD Suite Nov 1, 2024
JoerivanEngelen added a commit that referenced this issue Nov 8, 2024
# Description
This merges MetaSWAP refactoring feature branch into master. Fixes merge
conflicts.

This consolidates the work in
#1255

# Checklist
<!---
Before requesting review, please go through this checklist:
-->

- [ ] Links to correct issue
- [x] 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

1 participant