Skip to content

Issue #863 transport regridding#907

Merged
luitjansl merged 14 commits into
masterfrom
issue_#863_transport_regridding
Mar 6, 2024
Merged

Issue #863 transport regridding#907
luitjansl merged 14 commits into
masterfrom
issue_#863_transport_regridding

Conversation

@luitjansl

Copy link
Copy Markdown
Contributor

Fixes #863

Description

transport models now can be regridded,

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 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have some suggestions, approving in advance.

Comment thread imod/mf6/adv.py Outdated
"""

from copy import deepcopy
from typing import Tuple

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since python 3.10, this is not necessary anymore, and you can use the the regular types tuple, list, str etc. for type annotations. We haven't updated this everywhere in the codebase yet. But this line can be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread imod/mf6/adv.py Outdated
Comment thread imod/mf6/package.py
Comment on lines +544 to +552
if hasattr(self,"auxiliary_data_fields"):
remove_expanded_auxiliary_variables_from_dataset(self)
for var in self.dataset.data_vars.keys():
if self._skip_masking_variable(var, self.dataset[var]):
masked[var] = self.dataset[var]
else:
masked[var] = self._mask_spatial_var(var, mask)

if hasattr(self,"auxiliary_data_fields"):
expand_transient_auxiliary_variables(self)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just an idea:

We could create a decorator to encapsulate functions/methods, where necessary, for removing aux vars and expanding them again. As this is also done in _skip_masking_variable, and maybe should be done for regridding/clipping as well? This is probably best done after we have done all tasks in this milestone. If you agree, we can make an issue for this and add it to the refactoring milestone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nice idea, created issue #909 for that.

Comment thread imod/mf6/simulation.py Outdated
screen_top=[0.0, 0.0],
screen_bottom=[-1.0, -1.0],
rate=[1.0, -2.0],
rate=[0.1, -0.2],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a comment how and why we departed from the original mf6 example

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done. note that the fixture did not mention the original mf6 example, so nothing was promised wrt that.
I added a paragraph to explain where the model in the fixture comes from and why it was changed.

luitjansl and others added 4 commits March 6, 2024 13:47
review comment

Co-authored-by: Joeri van Engelen <joerivanengelen@hotmail.com>
review comment

Co-authored-by: Joeri van Engelen <joerivanengelen@hotmail.com>
@luitjansl luitjansl added this pull request to the merge queue Mar 6, 2024
Merged via the queue into master with commit a7b376d Mar 6, 2024
@luitjansl luitjansl deleted the issue_#863_transport_regridding branch March 6, 2024 14:57
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.

add support for transport regridding

2 participants