Skip to content

Issue #1779 Implementation of MetaSWAP model splitter#1780

Open
verkaik wants to merge 4 commits intomasterfrom
issue_#1779_msw_model_split
Open

Issue #1779 Implementation of MetaSWAP model splitter#1780
verkaik wants to merge 4 commits intomasterfrom
issue_#1779_msw_model_split

Conversation

@verkaik
Copy link
Contributor

@verkaik verkaik commented Mar 11, 2026

Fixes #1779

Description

Implementation of MetaSWAP model splitter.

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
  • If feature added: Added feature to API documentation
  • If pixi.lock was changed: Ran pixi run generate-sbom and committed changes

@verkaik verkaik requested a review from JoerivanEngelen March 11, 2026 14:57
@verkaik verkaik added the enhancement New feature or request label Mar 11, 2026
@sonarqubecloud
Copy link

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've got some comments, of which 2 are reminders to myself to create an issue so you can ignore those.

from imod.typing import IntArray


class CouplerMapping(MetaSwapPackage):
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: We need to add a ClippablePackage and SplittablePackage interface, just like we have a RegriddingPackage interface. The fact that we need to add IPackageBase here shows how unclear things become without it. I'll open an issue for this.

f"Missing the following required packages: {missing_packages}"
)

meteo_set = pkg_types_included & set(METEO_PACKAGES)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

return msw_model


def msw_add_sprinkling(msw_model):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

if isinstance(pkg, GridData):
continue

if has_meteogrid_copy and isinstance(pkg, MeteoMapping):
Copy link
Contributor

Choose a reason for hiding this comment

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

What case is catched with has_meteogrid_copy?

A model can have a MeteoGrid, a MeteoGridCopy, or none of those.
In case of the latter, an error is thrown with the extra model check you added in this PR upon writing. In case MeteoGrid, an error is thrown as well. So I guess the has_meteogrid_copy doesn't really serve a purpose here?

sliced_index = sliced_grid_pkg.generate_index_array()

# Add package to model if it has data in the overlap.
if bool(sliced_index.any()):
Copy link
Contributor

Choose a reason for hiding this comment

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

Good that you are catching the case where a submodel is entirely outside the active domain. I found the name has_overlap a bit confusing, as I thought at first it meant to overlapping subdomains. Something like is_in_active_domain is clearer here.

)
return clipped

def split(
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: Probably good to create a separate place to put splitting logic of this method and the MODFLOW6Simulation method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Add MetaSWAP model splitter

2 participants