Intertidal exposure - Githubissues

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...?

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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 :)

review-notebook-app[bot] commented 2 months ago

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!

review-notebook-app[bot] commented 2 months ago

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 :)

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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!

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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