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

Bugfix subbasin #263

Merged
merged 7 commits into from May 7, 2024
Merged

Bugfix subbasin #263

merged 7 commits into from May 7, 2024

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Apr 11, 2024

Issue addressed

Fixes #236
Fixes #261

Explanation

Earlier error messages.

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.

@hboisgon hboisgon requested a review from dalmijn April 11, 2024 03:22
@dalmijn dalmijn requested a review from shartgring April 26, 2024 13:08
Copy link
Collaborator

@shartgring shartgring left a comment

Choose a reason for hiding this comment

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

Approved and code is good, but do check the comments on the description of the errors

hydromt_wflow/wflow.py Show resolved Hide resolved
hydromt_wflow/wflow.py Show resolved Hide resolved
hydromt_wflow/workflows/basemaps.py Outdated Show resolved Hide resolved
hydromt_wflow/wflow.py Outdated Show resolved Hide resolved
@dalmijn dalmijn requested a review from shartgring April 26, 2024 14:17
Copy link
Collaborator

@shartgring shartgring left a comment

Choose a reason for hiding this comment

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

Looks good! Code works, still some changes are needed related to the error messaging

Also checked on local machine for following scenarios:

  • build regular model, looks good
  • too coarse resolution -> error as expected
  • too high upa -> error as expected
  • weird location for subbasin outlet -> error as expected
  • missing streamflow info so too small model -> error as expected
  • small model -> warning as expected

@hboisgon hboisgon requested review from shartgring and removed request for dalmijn April 29, 2024 02:53
hydromt_wflow/wflow.py Show resolved Hide resolved
@hboisgon hboisgon merged commit e7a4ce2 into main May 7, 2024
6 checks passed
@hboisgon hboisgon deleted the bugfix_subbasin branch May 7, 2024 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants