earthlab-education/ea-lidar-uncertainty-review

Review Checklist - PR #24 @mthomp89

csandberg303 opened this issue · 0 comments

Review Checklist - PR #24 @mthomp89

CI Checks
The notebook runs from start to finish on all operating systems:
[x] Mac
[ ] Windows
[ ] Linux
I found the file wouldn't initially run on my Windows laptop, and it appears it also failed the Linux check. I tracked this issue to an
unsorted glob list in creating the initial single-scene HARV dataframe (and left a few specific comments in your code about this).

Reproducibility
[x] Are the data downloaded in the code
[x] Are paths created to ensure they work on all operating systems (using os.path.join)
[x] Are comments used to clarify the contents of the code that can’t be clarified using expressive variable and function names alone?
[ ] Does the notebook run from start to finish?
I tested the code after making the suggested corrections mentioned above, and was able to see the code complete to finish - NICE JOB!! I liked your use of Seaborn to create an attractive plot.

PEP 8 standards & Code Readability
Functions
[ ] Do functions follow PEP 8 format conventions?
I did notice some of the red comments within the function definition exceeded the PEP 8 line length
[x] Are function docstrings clear (all inputs and outputs clearly described and defined)
I thought your docstrings were succinctly clear, with the input and output parameters very well defined. I'd suggest possibly expanding some details after the one-line summary (such as how the 'mask_crop_ndvi' function will also mask and crop the ndvi output, in addition to simply calculating it.
[x] Are function names expressive (the name describes what the function does)?
[x] Are functions easy to understand and read?
[x] How many tasks does each function do?

Package imports
[x] Are standard modules (those included with the base python install) vs. third party (related but externally developed tools) import groups correct with appropriate spacing in between each group?
[x] Are variable names throughout the code, expressive?
[x] Is the code overall easy to understand and read? Are there things that would make it more clear / cleaner?
I missed seeing some extra Markdown cells to give additional clarity about the workflow, and this seems a bit nit-picky but I don't think the vector and landsat-crop paths need to be expressly defined, as the directory names could simply be typed into the glob function after 'site-path'.

DRY Code
[x] Are segments of code repeated in the file or is the code DRY?
[x] Are loops used to optimize DRY code?
[x] Are functions used to optimize DRY code?
[x] Are there any areas that could be potentially improved (you can suggest improvements OR you can just highlight parts of the code where you suspect it could be cleaner / more efficient.
Please note my comments about sorting the glob list in your 'sites_path' variable, but other than that I thought your workflow was pretty splendid!

Novel Approaches to Problem solving
[ ] Highlight any novel approaches to completing the assignment.
I thought your use of the np.nanmean function was a great solution to removing the nan values from the final dataframe 🥇 I noticed you got full points in the sanity check, for the dataframe having the correct number of masked values, WELL DONE!