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

Implement NoDataStrategy to deal with optional setup_functions #229

Merged
merged 6 commits into from
Feb 13, 2024

Conversation

JoostBuitink
Copy link
Collaborator

@JoostBuitink JoostBuitink commented Jan 11, 2024

Issue addressed

Fixes #208, but needs Deltares/hydromt#731 to be fixed in order to work.

Explanation

Implemented a NoDataStrategy that ignores errors for the optional setup function (lakes, reservoirs, glacier and gauges). Now a warning is returned (in stead of an error), similar to the old behavior of hydromt_wflow. However, as the NoDataStrategy is currently incomplete in HydroMT-core, we need Deltares/hydromt#731 to be fixed before this fix works.

Checklist

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

@JoostBuitink JoostBuitink added the bug Something isn't working label Jan 11, 2024
@JoostBuitink JoostBuitink marked this pull request as draft January 11, 2024 16:31
@JoostBuitink
Copy link
Collaborator Author

Stable tests are failing, because I believe that they do no yet use the latest version of hydromt (v0.9.3).

@JoostBuitink JoostBuitink marked this pull request as ready for review February 8, 2024 17:12
Copy link
Collaborator

@dalmijn dalmijn 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 the good work! LGTM. I'd only request that setup_reservoirs is structured the same in this regard as the other methods.

Comment on lines 1695 to 1696
else:
self.logger.info("Skipping method, as no data has been found")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say, for consistency's sake, make this the same as the other methods, i.e.:

if ... is None:
    logger.warning(< warning message >)
    return

@Starphire1
Copy link

I'm excited about a new release. It would be great to have water level management and inflow/outflow calculations for lakes/reservoirs (if possible). It would also be nice if the created toml file at model build stage includes, e.g. lake = "wflow_lakelocs" and reservoir = "wflow_reservoirlocs" to reduce a manual model build step. The reservoir model build has been an exciting new addition to the model. Thanks for all the updates!

@dalmijn dalmijn self-requested a review February 13, 2024 09:27
Copy link
Collaborator

@dalmijn dalmijn left a comment

Choose a reason for hiding this comment

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

LGTM.

@dalmijn dalmijn merged commit f4a6486 into main Feb 13, 2024
4 of 6 checks passed
@JoostBuitink JoostBuitink deleted the fix_nodata branch February 14, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility with nodata requirements of core
3 participants