-
Notifications
You must be signed in to change notification settings - Fork 0
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 tests for ewatercycle 2.3.1 #9
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -169,8 +171,7 @@ def test_saved_yaml_content(self, forcing, tmp_path): | |||
|
|||
def test_saved_yaml(self, forcing, tmp_path): | |||
saved_forcing = WflowForcing.load(tmp_path) | |||
# shape should is not included in the yaml file | |||
forcing.shape = None | |||
forcing.shape = forcing.directory / "Rhine.shp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shape is now copied upon saving, so this behavior changed.
src/ewatercycle_wflow/forcing.py
Outdated
@@ -28,6 +28,7 @@ class WflowForcing(DefaultForcing): | |||
Inflow (str) = None: Variable name of inflow data in input file. | |||
""" | |||
|
|||
filenames: dict[str, str] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to leave this empty as wflow (old python version) requires a single netcdf for multiple variables. It had to be included though to follow the parent class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird that extending needs you to repeat the properties, oh well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not sure, didn't have to do this with Hype. I'll double check...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it runs fine without that line now 🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
src/ewatercycle_wflow/forcing.py
Outdated
@@ -28,6 +28,7 @@ class WflowForcing(DefaultForcing): | |||
Inflow (str) = None: Variable name of inflow data in input file. | |||
""" | |||
|
|||
filenames: dict[str, str] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird that extending needs you to repeat the properties, oh well.
This reverts commit f947035.
Didn't need to bump the version: I only modified the tests. |
Closes #8
I re-rendered the forcing notebook.
The model.ipynb notebook ran without issues.