Closed erialC-P closed 1 year ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
I'll leave formal review for those more experienced with the relevant libraries. It looks great though!
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:16Z ----------------------------------------------------------------
I don't think it's a major issue, but the other existing DEA_products notebooks basically copy directly from the first page of CMI, e.g. "Background", "What this product offers", "Applications", "Publications":
https://docs.dea.ga.gov.au/notebooks/DEA_products/DEA_Water_Observations.html#Background
Might be worth checking if there's anything from there that we could easily bring across to make sure we have consistency between CMI and this notebook; at least the "Publications" would be good to include I think
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:16Z ----------------------------------------------------------------
Line #7. # import xarray as xr
Can we remove xarray here?
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:17Z ----------------------------------------------------------------
Use the same headings from the other DEA_products notebooks, e.g. https://docs.dea.ga.gov.au/notebooks/DEA_products/DEA_Water_Observations.html#Available-products-and-measurements
"Available products and measurements"
"List products available in Digital Earth Australia"
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:18Z ----------------------------------------------------------------
Add "List measurements" subheading above
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:19Z ----------------------------------------------------------------
Add "Loading data" heading (e.g. https://docs.dea.ga.gov.au/notebooks/DEA_products/DEA_Water_Observations.html#Loading-data), then make "Select and view your study area" a subheading beneath
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:19Z ----------------------------------------------------------------
Line #18. Video("DEA_Mangroves.mp4", embed=True)
I don't think we need to actually attach the MP4 file in this pull request - the notebook should embed it directly. I'd remove it if there's not a good reason to include it
Also, the Video
thing is really nice!
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:20Z ----------------------------------------------------------------
Can we add an "Example applications" major heading here (e.g. https://docs.dea.ga.gov.au/notebooks/DEA_products/DEA_Water_Observations.html#Example-application:-mapping-inundation-frequency-and-tracking-changes-in-surface-water-over-time), and have the following examples as subheadings below it?
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:21Z ----------------------------------------------------------------
This is so cool!
View / edit / reply to this conversation on ReviewNB
robbibt commented on 2023-09-11T07:19:22Z ----------------------------------------------------------------
Also such a cool application. My only complaint: can we choose a different colourscheme here? This one isn't colorblind friendly, and it's linear data so doesn't need a diverging colourscheme.
I reckon simply something like "Greens" or "YlGn" would be easier to interpret
Thanks for the review @robbibt! Super useful feedback and comments - I'm happy to accept them all!
Awesome - just realised this will need to be added to the README.rst
file for it to render in the docs too.
And I don't think I hit commit on deleting the MP4 file on my end - can you delete it in your branch? (just deleting the file then committing the deleted file should do it)
Then I think we're good to go!
Proposed changes
This is a new notebook for inclusion with the other DEA_products notebooks, introducing the DEA Mangroves dataset.
Closes issues
Checklist (replace
[ ]
with[x]
to check off)Load packages
General advice
)jupyterlab_code_formatter
tool can be used to format code cells to a consistent style: select each code cell, then clickEdit
and then one of theApply X Formatter
options (YAPF
orBlack
are recommended).NCI
andDEA Sandbox
(flag if not working as part of PR and ask for help to solve if needed)Notebook currently compatible with the NCI|DEA Sandbox environment only
line below the notebook title to reflect the environments the notebook is compatible withThis notebook has not been tested for use on the NCI.
Note: when plotting the timeseries statistics in this notebook, I opted to leave in some commented out code which refers to a mostly insignificant extra class in the dataset
not_observed
. Users can optionally include this class in their output plots if they like. However, I'm open to removing this from the notebook for simplicity.Thanks for reviewing! :)