Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[WIP] Notebook - general-exploration-landcover_io #110

Merged
merged 16 commits into from
Sep 24, 2022

Conversation

acocac
Copy link
Member

@acocac acocac commented Aug 10, 2022

Before review

  • Work in progress
  • Ready for review
  • Need help!

Description

Add the executable notebook Exploring Land Cover Data (Impact Observatory) (see the notebook idea in issue #99).

Motivation and Context

New notebook contribution to the exploration theme.

How has this been tested?

GitHub actions deployed the forked template by the main author, @jamesdamillington, then the repo was transferred to the EnvDS book organisation (see https://github.com/Environmental-DS-Book/general-exploration-landcover_io).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • New notebook and/or content
  • Documentation notebook/content update
  • Other (please describe):
  • Feature change (upgrade version)

Checklist:

  • My code follows the code style of this project: Python
  • I have read the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests to cover my changes
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage)

Additional information

@netlify
Copy link

netlify bot commented Aug 10, 2022

Deploy Preview for the-environmental-ds-book ready!

Name Link
🔨 Latest commit 9136fa6
🔍 Latest deploy log https://app.netlify.com/sites/the-environmental-ds-book/deploys/632dc9faf6ca4d000899f276
😎 Deploy Preview https://deploy-preview-110--the-environmental-ds-book.netlify.app/gallery/exploration/general-exploration-landcover_io/general-exploration-landcover_io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@acocac acocac added exploration Exploration Notebooks high priority notebook Add/update notebook needs-review labels Aug 10, 2022
@acocac acocac linked an issue Aug 10, 2022 that may be closed by this pull request
9 tasks
@acocac acocac mentioned this pull request Aug 11, 2022
9 tasks
@acocac
Copy link
Member Author

acocac commented Sep 23, 2022

@jamesdamillington the proof of the notebook is ready for your revision.

Please feel free it the Netlify preview box above or click here.

To reduce the length of the notebook, all cells with static/interactive plots and others with considerable code have the Click to show button. Note this only hides cells in the html version and not when you open the file in Binder or locally.

Please confirm and provide comments where necessary:

  • Does the proof match the original and revised content of the notebook?
  • Are Binder and RoHub badges within the preview working?
  • Are the proposed Click to show buttons ok for you? If not, which ones do you suggest removing?

(optional) @annefou @aedebus can you confirm if you have any comments of the proof?

@jamesdamillington
Copy link
Collaborator

jamesdamillington commented Sep 23, 2022

@acocac Thanks for all your work pulling the proof together! It all looks good to me except for one thing: in Section 3 the Tip box says

We could also do an interactive plot with hvplot for Pandas data (not shown here to reduce notebook filesize - uncomment to run yourself).

But the code was not commented and so the interactive plot does show in the notebook...

We should either remove the Tip box (preferable I think) or comment the code out and re-compile so the hvplot doesn't appear.

Everything else looks okay (Binder takes a long time to boot but once it has, everything seems to work okay)

@acocac
Copy link
Member Author

acocac commented Sep 23, 2022

@jamesdamillington thanks for the quick reply!
I'm keeping the tip box, and just rephrasing the tip to:
We could also do an interactive plot with hvplot for Pandas data (see the cell below).
Let me fix it now!

@acocac
Copy link
Member Author

acocac commented Sep 23, 2022

@jamesdamillington can you double check the preview? it should have the suggested change in the tip box.

May I ask if you can complete the checkbox of my previous comment. This is a sort of final confirmation that the notebook is ok to go (:

@jamesdamillington
Copy link
Collaborator

Hi @acocac Thanks for the edit. Everything looks great and good to go, but how do I check the boxes?

@acocac
Copy link
Member Author

acocac commented Sep 23, 2022

Hi @acocac Thanks for the edit. Everything looks great and good to go, but how do I check the boxes?

Cool - Thanks for confirming all looks good to you! Regarding the checking boxes, you can copy them into a new comment and check them in case you cannont click-on each of them at my comment.

@jamesdamillington
Copy link
Collaborator

jamesdamillington commented Sep 23, 2022

  • Does the proof match the original and revised content of the notebook?
  • Are Binder and RoHub badges within the preview working?
  • Are the proposed Click to show buttons ok for you?

@acocac

@acocac
Copy link
Member Author

acocac commented Sep 24, 2022

Excellent.

  • Does the proof match the original and revised content of the notebook?
  • Are Binder and RoHub badges within the preview working?
  • Are the proposed Click to show buttons ok for you?

@acocac

Cool - thanks for confirming all is ok.

Apart from the nice and well-documented sections of the notebook, I've learnt how to properly cross-reference them which would be a very useful tip to recommend in the WIP PR#115 on improved guidelines.

I hope the publication process has been a positive experience for you and reviewers!

@acocac acocac merged commit 7f34501 into master Sep 24, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
exploration Exploration Notebooks high priority needs-review notebook Add/update notebook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NBI] Exploring Land Cover Data
2 participants