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

Update methods around gauges #172

Merged
merged 18 commits into from Jun 13, 2023
Merged

Update methods around gauges #172

merged 18 commits into from Jun 13, 2023

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Mar 30, 2023

Based on several open issues, this PR contains several changes associated to gauges:

@hboisgon hboisgon added enhancement New feature or request setup methods Issue related to existing or new setup methods labels Mar 30, 2023
@hboisgon hboisgon self-assigned this Mar 30, 2023
@hboisgon
Copy link
Contributor Author

I think this PR is more or less ready to be merged. Because I added a new create_gauges method, I wonder if we include it in the next release or wait for this issue to be fixed in hydromt core before merging: Deltares/hydromt#312

Copy link
Contributor

@laurenebouaziz laurenebouaziz left a comment

Choose a reason for hiding this comment

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

Really nice work @hboisgon !!!
I have only some very very minor suggestions, more in terms of additional details in the documentation.

I tested the new snapping option based on area using data from a project and it works very well, nice implementation! also great that there are now more possibilities for the gauges_fn format.

the tests are also running for me, so I think after checking these minor comments, it is ready to merge!

hydromt_wflow/wflow.py Show resolved Hide resolved
hydromt_wflow/wflow.py Show resolved Hide resolved
hydromt_wflow/wflow.py Outdated Show resolved Hide resolved
hydromt_wflow/wflow.py Outdated Show resolved Hide resolved
hydromt_wflow/wflow.py Show resolved Hide resolved
hydromt_wflow/wflow.py Show resolved Hide resolved
hydromt_wflow/wflow.py Show resolved Hide resolved
@hboisgon
Copy link
Contributor Author

Hi @laurenebouaziz ! Thanks a lot for the review! I added better docstrings, you were absolutely right :) and I answered some of your questions here too. Could you re-review and see if the changes I made are good?

@laurenebouaziz
Copy link
Contributor

thanks @hboisgon for the clear docstrings and all the explanations to the questions I had, all clear to me. Apart from the last tiny comment, I think it is good to merge!

Copy link
Contributor

@laurenebouaziz laurenebouaziz left a comment

Choose a reason for hiding this comment

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

Great, thanks @hboisgon for all these nice new additions!

@laurenebouaziz laurenebouaziz merged commit 926ca96 into main Jun 13, 2023
2 checks passed
@laurenebouaziz laurenebouaziz deleted the gauges branch June 13, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request setup methods Issue related to existing or new setup methods
Projects
None yet
3 participants