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

Issue #1255 msw refactor mf6 obj at write #1259

Open
wants to merge 12 commits into
base: msw_refactoring_feature_branch
Choose a base branch
from

Conversation

JoerivanEngelen
Copy link
Contributor

@JoerivanEngelen JoerivanEngelen commented Oct 29, 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

  • 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

@JoerivanEngelen JoerivanEngelen changed the base branch from master to msw_refactoring_feature_branch October 29, 2024 17:12
Copy link

sonarcloud bot commented Oct 30, 2024


def _render(self, file, index, svat: xr.DataArray):
self._create_mod_id_rch(svat)
# package check only possible after calling _create_mod_id_rch
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

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])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

self,
directory: Union[str, Path],
mf6_dis: StructuredDiscretization,
mf6_wel: Mf6Wel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean there can only be one well in the model?

def __init__(
self,
max_abstraction_groundwater: xr.DataArray,
max_abstraction_surfacewater: xr.DataArray,
well: Mf6Wel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disentangle Modflow objects as much from the MetaSWAP objects as possible
2 participants