Add Species Distribution Modeling Tutorial

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:31Z ----------------------------------------------------------------

Author needs to be your GitHub ID (osgeokr)

jdbcode commented on 2024-03-13T20:24:30Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:32Z ----------------------------------------------------------------

I suggest you choose one visualization of the data - my preference is the heatmap. Since they both convey the same info, it is redundant and a little confusing. Maybe I'm missing an important point though?

jdbcode commented on 2024-03-13T20:33:05Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:33Z ----------------------------------------------------------------

For style consistency, please use "snake_case" for variable names throughout.

jdbcode commented on 2024-03-13T21:21:17Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:34Z ----------------------------------------------------------------

Since GrainSize is a parameter, I wouldn't mention 1km in the code comment.
I wonder about hard coding the scale argument in sampleRegions - my intuition says that it should either be set by GrainSize or left blank to default to scale of image's first band (this is less explicit though)
Data var name should be lowercase for style consistency
Continue the rand_point_vals line on a second line to get line length closer to 80 char

jdbcode commented on 2024-03-13T20:38:40Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:34Z ----------------------------------------------------------------

Try to reduce line length closer to 80 char
Suggest to use zoom 14 so we can see the full extent of the 1km cells that intersect the points

jdbcode commented on 2024-03-13T20:39:27Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:35Z ----------------------------------------------------------------

Suggest to chain all methods to avoid defining MedianTCC twice. Also nit reminder of preferred var name style as all lower snake_case

MedianTCC = TCC.filterDate('2000-01-01', '2015-12-31').select(
['tree_canopy_cover'], ['TCC']).median()

_jdbcode commented on 2024-03-13T21:02:27Z_ ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:36Z ----------------------------------------------------------------

Was setting tileScale to 16 necessary to avoid memory issues? If not, it would be best practice to leave it as default. (I tried using the default and it was successful, but I had just run the analysis with tileScale=16 so I may have benefited from cached data)

jdbcode commented on 2024-03-13T21:03:31Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:37Z ----------------------------------------------------------------

Including the cell values text makes the graphic really busy and I little hard to interpret and the text is quite small and sort of concatenated. I think it might be better to not include the individual cell labels. Maybe the labels are optional with a boolean parameter, because they are nice in the filtered var plot.

jdbcode commented on 2024-03-13T21:04:44Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:37Z ----------------------------------------------------------------

Please try to get line lengths closer to 80 char
Consider using the ee.Reducer.minMax() instead of min and max separately - I believe it works as a single pass so should be a bit faster and less code

jdbcode commented on 2024-03-13T21:07:22Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:38Z ----------------------------------------------------------------

Same as above: reduce line length, consider minMax reducer

jdbcode commented on 2024-03-13T21:08:40Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:39Z ----------------------------------------------------------------

Try to reduce line length to near 80 char

jdbcode commented on 2024-03-13T21:08:54Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:40Z ----------------------------------------------------------------

I don't think setting tileScale is necessary here, unless I'm missing something. The example in this demo works with the default, but maybe tileScale is needed for more broad use and application of this workflow?

jdbcode commented on 2024-03-13T21:10:38Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:41Z ----------------------------------------------------------------

Casting GrainSize as ee.Number is unnecessary in this case.

jdbcode commented on 2024-03-13T21:11:02Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:42Z ----------------------------------------------------------------

I think you can use ee.Geometry.coveringGrid to create this, which I think is more intuitive for readers. I.e. no need for this function, and in the next cell, I think the grid FeatureCollection can be defined as such:

Grid = watermask.reduceRegions(
    collection=AOI.coveringGrid(scale=Scale, proj='EPSG:4326'),
    reducer=ee.Reducer.mean()).filter(ee.Filter.neq('mean', None))

_jdbcode commented on 2024-03-13T21:20:21Z_ ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:42Z ----------------------------------------------------------------

Try to get line lengths close to 80 char

jdbcode commented on 2024-03-13T21:13:12Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:43Z ----------------------------------------------------------------

Recommend not using \ for line continuation - how about:

images = ee.List.sequence(
    0, ee.Number(numiter).multiply(4).subtract(1), 4).map(
    lambda x: results.get(x))

_jdbcode commented on 2024-03-13T21:14:05Z_ ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:44Z ----------------------------------------------------------------

Same note about avoding \

jdbcode commented on 2024-03-13T21:14:25Z ----------------------------------------------------------------

Done

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

View / edit / reply to this conversation on ReviewNB

jdbcode commented on 2024-02-20T22:57:45Z ----------------------------------------------------------------

The graph looks so nice and you include the value label - so I think you should not also print importance in the for loop.

jdbcode commented on 2024-03-13T21:15:45Z ----------------------------------------------------------------

Done

jdbcode commented 7 months ago

@osgeokr you've done an amazing job! This is one of the best tutorials I reviewed and the analyses is really useful. Thank you!

I left some comments that are mostly nits about code formatting and style and a couple suggestions for alternative implementations. You should be able to see comments in context using the ReviewNB links above.

Common formatting and style nits:

Aim for 80 char line length (some are too long)
Aim for consistency with variable name (best to use snake_case)
Remove all outputs

The Black Python code formatter is good for line length fixing - there is a command line version (I don't think it works on notebooks though), but there is a UI for it that is nice for copy/paste as few blocks of code: https://black.vercel.app/ (access the settings in the lower left to change line length to 80 char - doesn't have to be exactly, but when we publish on DevSite, 80 char fits without overflow scroll)

Please address the comments or reply to them for discussion or clarification. Once all the comments are addressed, please commit your changes and I'll look again.

Thanks, Justin

osgeokr commented 7 months ago

Thank you sincerely for your meticulous review! Working on the tutorial has enhanced my understanding of GEE, for which I am grateful. I have quickly gone through all the reviews you provided and have no further objections, so I will proceed to revise and commit the changes accordingly. I believe it will be possible within this week.

osgeokr commented 7 months ago

I have committed the updated file with the modifications to the comments you provided. Thanks a lot!

jdbcode commented 7 months ago

@osgeokr - thank you for the updates! I should be able to look in the next few days.