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

Intertidal exposure #1261

Merged
merged 8 commits into from
Aug 19, 2024
Merged

Intertidal exposure #1261

merged 8 commits into from
Aug 19, 2024

Conversation

erialC-P
Copy link
Collaborator

@erialC-P erialC-P commented Aug 8, 2024

Proposed changes

This PR adds a new notebook to the Real_world_examples suite and demonstrates customisation options that are available when undertaking intertidal exposure analysis.
The notebook is a bit slow to run in some places, with the whole runtime being approximately 6 - 10 minutes. Much of this speed is accounted for in the use of the exposure function which runs very high resolution tidal modelling in the background. However, some of the lagginess may be related to the use of animations/gifs in the notebook.

Checklist

(Replace [ ] with [x] to check off)

  • Notebook created using the DEA-notebooks template
  • [x ] Remove any unused Python packages from Load packages
  • [x ] Remove any unused/empty code cells
  • [x ] Remove any guidance cells (e.g. General advice)
  • [x ] Ensure that all code cells follow the PEP8 standard for code. The jupyterlab_code_formatter tool can be used to format code cells to a consistent style: select each code cell, then click Edit and then one of the Apply X Formatter options (YAPF or Black are recommended).
  • [x ] Include relevant tags in the final notebook cell (refer to the DEA Tags Index, and re-use tags if possible)
  • [x ] Clear all outputs, run notebook from start to finish, and save the notebook in the state where all cells have been sequentially evaluated
  • Test notebook on both the NCI and DEA Sandbox (flag if not working as part of PR and ask for help to solve if needed)
  • If applicable, update the Notebook currently compatible with the NCI|DEA Sandbox environment only line below the notebook title to reflect the environments the notebook is compatible with
  • [x ] Check for any spelling mistakes using the DEA Sandbox's built-in spellchecker (double click on markdown cells then right-click on pink highlighted words). For example:

sandbox_spellchecker

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:26Z
----------------------------------------------------------------

Line #4.    !pip install sunriset

Is this necessary? It should be installed when we install Intertidal (it's included in the setup.py and requirements.in)


erialC-P commented on 2024-08-15T01:54:06Z
----------------------------------------------------------------

You're right. This was legacy code. Deleted :)

erialC-P commented on 2024-08-19T03:30:52Z
----------------------------------------------------------------

Actually...with a fresh kernel, it turns out that I did need to keep this line. I wonder if it's because it's a non-standard package...?

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:27Z
----------------------------------------------------------------

Line #8.    HTML("Jupyter.notebook.kernel.restart()")

What is this for?


erialC-P commented on 2024-08-15T01:57:04Z
----------------------------------------------------------------

On further testing, it's totally redundant as it doesn't work. I was trying to automatically restart the kernel following the pip install. Will test whether a restart is even required.

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:27Z
----------------------------------------------------------------

Line #2.    %cd ../Supplementary_data/Customising_intertidal_exposure

I'd probably avoid doing this, and instead update the output paths where you export results (e.g. add "../Supplementary_data/..." to the output paths).

I'm torn about whether we just want to save those outputs directly in "Real_world_examples", or in that "Supplementary_data" folder instead. The second option is probably cleaner but not something we've done before in other notebooks...


erialC-P commented on 2024-08-15T02:15:04Z
----------------------------------------------------------------

good point. I've removed that line and updated figure outputs into the R_W_E folder

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:28Z
----------------------------------------------------------------

"Load data"

(some headings use "All Starting Capitals" and others use "Just the first capital" - should standardise for consistency with other notebooks.

erialC-P commented on 2024-08-15T02:00:30Z
----------------------------------------------------------------

fixed :)

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:29Z
----------------------------------------------------------------

Line #3.    ds = dc.load(product="ga_s2ls_intertidal_cyear_3", **query_params)

Do we need to load all DEA Intertidal layers here? Or could we just load "elevation" and "exposure"? If so, that could improve load times quite a bit


erialC-P commented on 2024-08-15T02:03:52Z
----------------------------------------------------------------

great suggestion!

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:29Z
----------------------------------------------------------------

Line #16.    # Add the geomad data layers into the master dataset

I'd be tempted to leave these as separate datasets, just easier to manage that way


erialC-P commented on 2024-08-15T05:31:14Z
----------------------------------------------------------------

Done :)

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:30Z
----------------------------------------------------------------

Line #45.    path = '/home/jovyan/dea_intertidal/dea-intertidal'

This points to your copy of the DEA Intertidal repo which users running this notebook won't have access to. We should use a relative link here so that the data gets saved next to or near this notebook (e.g. inside "Real_world_examples", or "Supplementary_data")


erialC-P commented on 2024-08-15T02:16:25Z
----------------------------------------------------------------

Yep - since found that little bug too. Updated to os.getcwd() which should set all figure outputs to the R_W_E folder alongside the notebook

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:31Z
----------------------------------------------------------------

Line #59.    Image(gif_path)

I do think this code block would be a natural candidate for a function that we could call from dea_tools.coastal! Would help hide away a fair bit of code and led to a simpler user experience, as well as providing a nice tool that could be run in different locations too. But we could always do that down the track.


erialC-P commented on 2024-08-15T02:17:47Z
----------------------------------------------------------------

Yeah, definitely open to that! Wasn't sure if this figure style was a bit niche to this application but a func would allow it to be very customisable!

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:31Z
----------------------------------------------------------------

Line #14.        modelled_freq = '30 min',

If this notebook takes a while to run, I would be very tempted to reduce this to something far less short - e.g. "3 h" perhaps. You could add this as a parameter up top, and add an explanatory note that we're using a low frequency for demonstration purposes, but for real analyses you'd use something shorter


erialC-P commented on 2024-08-15T04:55:22Z
----------------------------------------------------------------

Yep! I agree that this is a good idea. Testing with a 2 H frequency gives results consistent with the notebook narrative so I'll reduce the frequency and add a note above with advice about freq selection.

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:32Z
----------------------------------------------------------------

Line #15.        modelled_freq = '30 min',

Same comment here - I reckon use a longer freq as the default, but allow the user to change this once they've run the notebook for the first time


Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:33Z
----------------------------------------------------------------

Line #36.    exp_counts = {"Exp >= 70%": (np.array(exp70_100)),#*0.0001,

Are these code comments meant to be here?


erialC-P commented on 2024-08-15T05:24:05Z
----------------------------------------------------------------

Nope! More legacy code :D

Copy link

review-notebook-app bot commented Aug 13, 2024

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2024-08-13T03:33:33Z
----------------------------------------------------------------

Line #81.    plt.show()

This figure is so cool!


erialC-P commented on 2024-08-15T05:24:34Z
----------------------------------------------------------------

Thanks! It tells a pretty cool story :)

@robbibt
Copy link
Member

robbibt commented Aug 13, 2024

@erialC-P I've just done a quick skim through for now, but this is fantastic... feels almost like an interactive/runnable version of a scientific paper, which is really neat and perfect for a Real world example I think! 🤩

I've suggested some small changes - I might jump into the branch and make some minor code suggestions at a later point too. For now, my only "more general" comment would be to go through and run either Black or YAPF code formatting on all the code cells so everything is nice and consistent - I think some got missed earlier.

Copy link
Collaborator Author

You're right. This was legacy code. Deleted :)


View entire conversation on ReviewNB

Copy link
Collaborator Author

On further testing, it's totally redundant as it doesn't work. I was trying to automatically restart the kernel following the pip install. Will test whether a restart is even required.


View entire conversation on ReviewNB

Copy link
Collaborator Author

fixed :)


View entire conversation on ReviewNB

Copy link
Collaborator Author

great suggestion!


View entire conversation on ReviewNB

Copy link
Collaborator Author

good point. I've removed that line and updated figure outputs into the R_W_E folder


View entire conversation on ReviewNB

Copy link
Collaborator Author

Yep - since found that little bug too. Updated to os.getcwd() which should set all figure outputs to the R_W_E folder alongside the notebook


View entire conversation on ReviewNB

Copy link
Collaborator Author

Yeah, definitely open to that! Wasn't sure if this figure style was a bit niche to this application but a func would allow it to be very customisable!


View entire conversation on ReviewNB

Copy link
Collaborator Author

Yep! I agree that this is a good idea. Testing with a 2 H frequency gives results consistent with the notebook narrative so I'll reduce the frequency and add a note above with advice about freq selection.


View entire conversation on ReviewNB

Copy link
Collaborator Author

Nope! More legacy code :D


View entire conversation on ReviewNB

Copy link
Collaborator Author

Thanks! It tells a pretty cool story :)


View entire conversation on ReviewNB

Copy link
Collaborator Author

Done :)


View entire conversation on ReviewNB

Copy link
Collaborator Author

Actually...with a fresh kernel, it turns out that I did need to keep this line. I wonder if it's because it's a non-standard package...?


View entire conversation on ReviewNB

Copy link
Member

@robbibt robbibt left a comment

Choose a reason for hiding this comment

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

Amazing!

@erialC-P erialC-P merged commit 7e29a84 into develop Aug 19, 2024
1 check passed
@erialC-P erialC-P deleted the intertidal_exposure branch August 19, 2024 05:28
geoscience-aman added a commit that referenced this pull request Sep 4, 2024
* Fix deprecation warnings and speed up code

* Change last modified date

* Intertidal exposure (#1261)

* New files for intertidal exposure notebook

* minor editing to Load packages cells

* Updated discord and removed slack links

* Improved markdown linking to image and gif in Introduction

* Incorporated SS and MA reviews

* Updated to include RBT reviews

* Minor pip install and notebook naming edits. Add notebook to README

* adding renamed exposure notebook back into PR

* adding global SAR access through microsoft planetary compute (#1263)

* adding global SAR access through microsoft planetary compute

* Make minor spelling and formatting amendments.

* small changes for PR

---------

Co-authored-by: geoscience-aman <96451725+geoscience-aman@users.noreply.github.com>

---------

Co-authored-by: Aman Chopra <aman.chopra@ga.gov.au>
Co-authored-by: geoscience-aman <96451725+geoscience-aman@users.noreply.github.com>
Co-authored-by: ClaireP <claire.phillips@ga.gov.au>
Co-authored-by: Alex Bradley <55119000+abradley60@users.noreply.github.com>
robbibt added a commit that referenced this pull request Oct 9, 2024
* Fix deprecation warnings and speed up code

* Change last modified date

* Intertidal exposure (#1261)

* New files for intertidal exposure notebook

* minor editing to Load packages cells

* Updated discord and removed slack links

* Improved markdown linking to image and gif in Introduction

* Incorporated SS and MA reviews

* Updated to include RBT reviews

* Minor pip install and notebook naming edits. Add notebook to README

* adding renamed exposure notebook back into PR

* adding global SAR access through microsoft planetary compute (#1263)

* adding global SAR access through microsoft planetary compute

* Make minor spelling and formatting amendments.

* small changes for PR

---------

Co-authored-by: geoscience-aman <96451725+geoscience-aman@users.noreply.github.com>

* Update USAGE.rst (#1268)

Add Swinburne course for 2024

* Minor compatibility change for tide modelling package (#1269)

* Mitch predict_xr (#1270)

* add probability array output to predict_xr

* predict_xr at proba_max args

* predict_xr match arg names

* xr_predict deal with multiband prob outout

* xr_predict merge output probs

* clean up comments and spacing

* Update USAGE.rst (#1272)

Add new reference, Burton et al 2024 Enhancing long-term vegetation monitoring in Australia: a new approach for harmonising the Advanced Very High Resolution Radiometer normalised-difference vegetation (NVDI) with MODIS NDVI

* Fix broken code on `unstable` Sandbox image (#1274)

* Updates for pyTMD

* Fix contours bug due to groupby squeeze

* Try loosening pyTMD requirements

* Update tests to pass on both stable and unstable sandbox

* Fix pansharpening bug

---------

Co-authored-by: Aman Chopra <aman.chopra@ga.gov.au>
Co-authored-by: geoscience-aman <96451725+geoscience-aman@users.noreply.github.com>
Co-authored-by: ClaireP <claire.phillips@ga.gov.au>
Co-authored-by: Alex Bradley <55119000+abradley60@users.noreply.github.com>
Co-authored-by: Bex Dunn <BexDunn@users.noreply.github.com>
Co-authored-by: Mitchell Lyons <mitchell.lyons@gmail.com>
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