Skip to content

Conversation

JoostBuitink
Copy link
Collaborator

@JoostBuitink JoostBuitink commented Mar 3, 2025

Issue addressed

Fixes #112

Explanation

With a flag, a user can specify if the clipping needs te be done "normally" (only saving all upstream area given a location), or "inversely" (removing the upstream area given a location). This is useful to link for example several separate wflow models, that together cover a large river basin.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@JoostBuitink JoostBuitink linked an issue Mar 3, 2025 that may be closed by this pull request
Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

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

Thanks for creating and updating the PR @JoostBuitink.
I like your solution too :)

Before merging, it would still be good to update the docstrings and also implement this to the clip_forcing method. Can you still do this? Else I'm happy to take over too, just let me know!

@JoostBuitink
Copy link
Collaborator Author

Thanks for creating and updating the PR @JoostBuitink. I like your solution too :)

Before merging, it would still be good to update the docstrings and also implement this to the clip_forcing method. Can you still do this? Else I'm happy to take over too, just let me know!

Ah forgot about the docstrings, will do that! Related to the clip_forcing, I understood that it uses the extent of the grid (staticmaps) to determine how the forcing should be clipped. So I don't think there are changes required there, is that right? I think the same holds for the clip_states function, but let me know if changes are needed in these functions as well!

Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

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

Of course you are right for forcing! Sorry I forgot how it was working... So all good now!
I was hesitating to add this change in the version 1 release. What do you think? If version 1 then we need to wait before we can merge.

@JoostBuitink
Copy link
Collaborator Author

Of course you are right for forcing! Sorry I forgot how it was working... So all good now! I was hesitating to add this change in the version 1 release. What do you think? If version 1 then we need to wait before we can merge.

No problem! I think this can be merged now (since it is also a very minor code change), such that for v1 changes it will also be included. Or do I overlook something?

@savente93 savente93 merged commit c3dc6c1 into main Mar 12, 2025
4 checks passed
@savente93 savente93 deleted the inverse_clipping branch March 12, 2025 14:47
savente93 added a commit that referenced this pull request Mar 17, 2025
* Add support for inverse clipping (removing upstream area from model)

* precommit fix

* add test

* precommit

* update docstring

* consistent naming

* update changelog

---------

Co-authored-by: Sam Vente <[email protected]>
@deltamarnix deltamarnix added this to the Release 0.8 milestone Apr 8, 2025
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.

inverse mask for model clipping
4 participants