Skip to content

Polish irrigation example notebook #180

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

Merged
merged 14 commits into from
Jul 19, 2021
Merged

Polish irrigation example notebook #180

merged 14 commits into from
Jul 19, 2021

Conversation

Peter9192
Copy link
Collaborator

@Peter9192 Peter9192 commented Jul 15, 2021

Using the updated API. Didn't change much about the notebook itself since it was already quite nice.

(branched off a previous PR so hence the many commits).

https://ewatercycle--180.org.readthedocs.build/en/180/examples/Irrigation.html

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@sverhoeven sverhoeven self-requested a review July 15, 2021 14:41
@@ -2,37 +2,47 @@
"cells": [
Copy link
Member

Choose a reason for hiding this comment

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

Can we add https://doi.org/10.5281/zenodo.1045339 link to standard dataset


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, great suggestion!

@@ -2,37 +2,47 @@
"cells": [
Copy link
Member

Choose a reason for hiding this comment

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

Instead of hardcoding lat/lon. Can we use grdc_latitude and grdc_longitude vars instead?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check

@@ -2,37 +2,47 @@
"cells": [
Copy link
Member

Choose a reason for hiding this comment

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

Hard to see that lines 1,4,5 are the concept due to lots of plotting code. Can the cell be split on line 6 and the comment be promoted to Markdown?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay

@@ -2,37 +2,47 @@
"cells": [
Copy link
Member

Choose a reason for hiding this comment

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

Instead of hardcoding lat/lon. Can we use grdc_latitude and grdc_longitude vars instead?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

Was able to run the notebook successfully.

Made some suggestions in reviewnb.

Might also be nice to use a lat/lon bounding box instead of index range ([31:41, 18:28]).

Spawned off #182

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@Peter9192
Copy link
Collaborator Author

@sverhoeven all suggestions applied except for set_as_xarray. Started in #185, but not yet ready for takeoff. Perhaps we should merge this already and add that later, is that okay with you?

@Peter9192 Peter9192 requested a review from sverhoeven July 16, 2021 10:19
Copy link
Member

@sverhoeven sverhoeven 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.

I like the description for running the experiment, much clearer.

@sverhoeven
Copy link
Member

@sverhoeven all suggestions applied except for set_as_xarray. Started in #185, but not yet ready for takeoff. Perhaps we should merge this already and add that later, is that okay with you?

Yep lets merge this PR and work on #185 later as its less critical.

@Peter9192 Peter9192 merged commit 7b3d61c into main Jul 19, 2021
@Peter9192 Peter9192 deleted the irrigation_notebook branch July 19, 2021 07:48
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.

2 participants