257 add smoothing functions #289
Conversation
|
I will review this today or tomorrow! When it comes to the linting issue, add |
|
Ahh, good to know 👀 It did 🐱🏍 Thank you 😎 |
There was a problem hiding this comment.
Hey, looks really good overall and the notebook is very illustrative! I noticed a few things regarding the style and docs that I pointed out in the comments.
One a bit bigger question: I noticed that the filters don't seem to necessarily require raster data, but actually any 2D Numpy data (so in other words, raster metadata is not needed). Do you think these functions still best fit under the raster processing category? Maybe more specifically I am thinking about the function signatures, so I propose we either do one of these modifications:
- Change raster inputs to Numpy array inputs
- Return the raster metadata in the (out_image, out_meta) format as other raster processing functions, even if the metadata is not modified
Additionally, please also include the .md files in the docs folder ;). I noticed these are missing for the surface derivatives too, so you can add both in this PR.
| Parameters: | ||
| raster: The input raster dataset. | ||
| method: The method to use for filtering. Can be either "mean" or "median". Default to "mean". | ||
| size: The size of the filter window. E.g., 3 means a 3x3 window. Default to 3. | ||
| shape: The shape of the filter window. Can be either "square" or "circle". Default to "circle". |
There was a problem hiding this comment.
This whole file seems to use smaller indentation than all other files. Change this to use 4 spaces.
| InvalidParameterValueException: If the filter size is smaller than 3. | ||
| If the filter size is not an odd number. | ||
| If the shape is not "square" or "circle". |
There was a problem hiding this comment.
We have so far formatted multiline docstring entries so that the next lines are indented one unit further than the first one, like this:
| InvalidParameterValueException: If the filter size is smaller than 3. | |
| If the filter size is not an odd number. | |
| If the shape is not "square" or "circle". | |
| InvalidParameterValueException: If the filter size is smaller than 3. | |
| If the filter size is not an odd number. | |
| If the shape is not "square" or "circle". |
If you could change all parameter and raises sections to follow this style, that would be good!
| shape: The shape of the filter window. Can be either "square" or "circle". Default to "circle". | ||
|
|
||
| Returns: | ||
| np.ndarray: The filtered raster array. |
There was a problem hiding this comment.
You can remove the np.ndarray from the beginning and simply have "The filtered raster array". Apply this change to the other functions in this file, too.
|
|
||
|
|
||
| @beartype | ||
| def get_kernel_size(sigma, truncate, size): |
There was a problem hiding this comment.
This function, and create_grid, gaussian_kernel and mexican_hat_kernel, all are missing parameter types and return types. Please add them.
| shape: The shape of the kernel. Can be either "square" or "circle". | ||
|
|
||
| Returns: | ||
| np.ndarray: The generated kernel. |
There was a problem hiding this comment.
You can remove the return type also here.
|
|
||
|
|
||
| @beartype | ||
| def _lee_additive_noise(window: np.ndarray, add_noise_var: Number) -> Number: |
There was a problem hiding this comment.
I would like all these private functions to have even just a short one line docstring that describes them a little bit – just so that we document our functions a bit more even if end users don't use these. No need to use the full args, returns, raises -template.
|
|
||
| Parameters: | ||
| raster: The input raster dataset. | ||
| size = The size of the filter window. E.g., 3 means a 3x3 window. Default to 3. |
There was a problem hiding this comment.
The = should be : here. This same typo is also in the following docstrings in this file.
There was a problem hiding this comment.
Happens when you lean on auto-docstring generation 😔 Sorry for those, I'll check these parts better before PR in the future 🕵️♂️
|
Ahoi Niko, thank you for the good review ⚡ Oh man, you're totally right. Missed the Will address this and any other point ASAP 🚀 Best, |
|
Ahoi Niko 🏴☠️ think I got everything on the requested changes list covered so far 🤗 The docs stuff was pretty new to me, when did we add that? 👀 However, the build marked some minor docstring inconsistencies in the derivatives module, which are contained in the PR as well. The notebook has been changed due to returning Let me know if there are other things to be changed 🐱👤 Cheers, Edit: |
|
Looks great now! ✨ It's basically ready to be merged, but I noticed one cell in the notebook didn't unpack the We have had the doc files since the beginning, but I have forgotten them from time to time myself and they haven't been discussed very frequently. However, it was only a couple months ago we started automatically building a PDF out of the docs and also publishing them here: https://gispocoding.github.io/eis_toolkit/ Regarding |
|
Thank you ✨ Notebook is ready, then let's get it merged 😎 Good to know this for the doc's 🦄 Pretty handy ⚙ |
Modularized smoothing functions with example notebook.
There is one
flake8error, that wants[x : x, y : y]with no whitespace between the letters ([x:x, y:y]). However, once corrected, black re-formats it to the version with whitespaces. May give an error in the GitHub actions.How we want to handle this?
Cheers,
Micha