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 #1164 cleanup utils #1172

Merged

Conversation

JoerivanEngelen
Copy link
Contributor

@JoerivanEngelen JoerivanEngelen commented Aug 23, 2024

Fixes #1164

Description

This implements the following cleanup utilities for the robin boundary conditions, namely the DRN, GHB, and RIV.

  • Cleanup helper functions, which are part of the public API (so that people could also use them outside MODFLOW 6)
  • Cleanup methods to the River, GeneralHeadBoundary, Drain package for convenience. These call the corresponding helper function. Save the user the hassle of manually finding the right arguments to clean up grids.
  • Add a utility method: Call function on grids, which separates grid variables from settings, calls a function on them, and then merges these dicts again.
  • Converted the fixtures in test_mf6_riv to regular functions, as they were used as such anyway. Without this, RivDisCases could not be used outside the test_mf6_riv module, now it can be imported.

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 marked this pull request as ready for review August 27, 2024 12:54
@JoerivanEngelen JoerivanEngelen requested review from luitjansl, Manangka and HendrikKok and removed request for luitjansl August 27, 2024 12:54
imod/mf6/drn.py Outdated Show resolved Hide resolved
imod/mf6/drn.py Outdated Show resolved Hide resolved
"""
dis_dict = {var: dis.dataset[var] for var in ["idomain"]}
cleaned_dict = self._call_func_on_grids(cleanup_drn, dis_dict)
super().__init__(cleaned_dict)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be self.__init__(cleaned_dict)?

I also don't really get what this line does. You are creating a new package but you don't use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The call to super().__init__(cleaned_dict), overrides the dataset attribute with a new dataset from dict. Just as we do in self.init. This method cleans up the dataset inplace, instead of returning a copy of a river object.

imod/mf6/riv.py Outdated Show resolved Hide resolved
grids["conductance"] = conductance.where(conductance > 0.0)
# Clip negative concentration cells to 0.0
if (concentration is not None) and not scalar_None(concentration):
grids["concentration"] = concentration.clip(min=0.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we inform the user by producing a warning? Or/and should we log this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I think using the standard_log_decorator should be sufficient. I added this to the functions. This can be a guidance to the API docs, which will provide more info on what is exactly cleaned up in the function.


- Removes wells where the screen bottom elevation exceeds screen top.
"""
deactivate = wel_ds["screen_top"] < wel_ds["screen_bottom"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

General for this file: We are making adjustemesnt to the model. Should we inform the user and/or log this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I added the standard_log_decorator to the cleanup utility functions, to log things.

Copy link

sonarcloud bot commented Sep 3, 2024

@JoerivanEngelen JoerivanEngelen merged commit 20e4bc2 into imod5_converter_feature_branch Sep 3, 2024
6 checks passed
@JoerivanEngelen JoerivanEngelen deleted the issue_#1164_cleanup_utils branch September 3, 2024 13:37
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.

2 participants