GeoscienceAustralia / TSCloudMask

1 stars 1 forks source link

Documentation and accessibility feedback #1

Closed robbibt closed 4 years ago

robbibt commented 4 years ago

Hey @pjt554 @LeoLym, here's some early comments relating to the documentation and accessibility of the TSmask code (I'll probably add some more later). Overall the code comments etc are really nice and clear and make it obvious what each line is doing - my suggestions focus mainly on moving function documentation into doc strings, moving functions out into an external script that is called from the notebook, and rearranging the remaining content with markdown headers to make things more clear:

  1. Include the function descriptions as doc strings within the functions themselves, rather than in markdown. For example: https://github.com/GeoscienceAustralia/dea-notebooks/blob/develop/Scripts/dea_datahandling.py#L120-L222
  2. Replace all instances of 'function IDs' e.g. F1, F2 with their function names instead e.g. load_s2_nbart_ts, output_ds_to_ENVI
  3. Move all functions to a stand-alone Python script, e.g. tsmask_funcs.py. This will make it much easier to track changes and suggest updates via Github, and also mean you can call the same functions from multiple notebooks (e.g. like the VDI and AWS notebook you have here)
  4. Import that script into notebook using import tsmask_funcs, and call funcs using tsmask_funcs.spatial_filter etc. If you add this code near the top of your notebook, any changes you make to the tsmask_funcs script will be automatically reloaded in the notebook without needing to restart the kernel:
    %load_ext autoreload
    %autoreload 2
  5. Rearrange notebook into the following order: a. All 'import modules' steps (including import tsmask_funcs) at top of the notebook below the introduction markdown b. Then 'Specify input parameters' c. Then remaining 'Main program' cells d. Use markdown cells for main program headings (e.g. '# Load surface reflectance data') to separate each main cell block in the main program and make it easier to follow
robbibt commented 4 years ago

Another minor one is that some of the functions have inconsistent spacing. e.g. :

image image

Would be good to quickly go through each of the functions and clean up the line spacing a bit. In Jupyterlab you can do it automatically by going Edit > Apply Black Formatter or Apply YAPF Formatter and it will re-format everything to be 100% consistent

robbibt commented 4 years ago

Hey @pjt554 , just one more thing that's more of a code suggestion than a documentation suggestion: both of the spatial_buffer and spatial_filter sections loop through each x and y pixel which is possibly quite slow. There's a few really handy morphological functions in scikit-image that might make this a lot faster as they work on the entire array in one go:

https://scikit-image.org/docs/dev/auto_examples/applications/plot_morphology.html

(e.g. Morphological "dilation" for buffering areas of pixels, morphological "opening" for cleaning isolated pixels)

pjt554 commented 4 years ago

The morphological fuctions do not offer flexibilities required by the spatial_buffer and spatial_filter functions. So I will keep them as current forms. However, I will keep investigating if there are similar functions with greater flexibilities in other packages.