Great, thanks! 😄

We have a memo page to work on someone else's PR here 😉: https://github.com/GlacioHack/xdem/wiki/Work-on-someone-else's-PR)

I only have a few comments:

• I'm not entirely sure if the current mean function works with NaNs, we should add a test checking the output exactly on a 3x3 array example, like shown in this comment: Move filter.py from xDEM to GeoUtils and integrate into raster class #691 (review). We should be able to replicate this test easily for all filters.
• Should we make Numba an optional dependency? Not sure we want to enforce it on users. Maybe we should do the same in xDEM actually.

For next PRs, we also need to:

• Open an issue to make the mean of the distance filter a variable,
• Open an issue to add other filters using the "Numba" engine.

We could also easily add an NMAD filter based on Numba (just mirroring the median filter code with Numba, with the NMAD calculation instead), which would solve this 4-year old issue! GlacioHack/xdem#69

We could add it to our "project" once created ?

@rhugonnet can you please check one last time, I think we might have some confusion with Numba ?

What confusion, can you explain in more details?

Great, thanks! 🙂
Just unsure what the confusion is about.

I thought you wanted numba to be mandatory, I don't know why, maybe because of two weeks hollidays, sorry ^^

Perfect! I see Numba is being annoying when solving in CI within the already-existing environment.

To fix this, the trick is to replace "graphviz" by "graphviz, numba" at this line to pre-install it:

But it means that we'll never be testing that the package functions normally without Numba in its environment.
This joins the issue of testing optional dependencies more robustly we had on the Profiling PR: GlacioHack/xdem#775 (review)
I'll open an issue on this.

Thanks @adebardo for continuing this long process of moving the filters! 😅

I made all my comments in-line but here are some major comments:

• I believe the implementation of the Gaussian filter with Nans is erroneous, unless I'm missing something?
• the expected array dimension and along which axes the filter is run should be clarified and homogeneous among filters (custom filters apply only along 2D, while scipy filters apply along all dimensions I believe).
• by checking scipy's documentation, I realize we may want to use vectorized_filter for filters on Nan arays. This may be a relatively new feature as I was not aware of it. Worth checking the performance against generic_filter.
• I think the tests are too weak and should test that the actual output content is correct on a few synthetic cases.

Looking at the previous PR and Romain's comments, it looks like point 1 and 3 may have be discussed already but I'm not sure what was the conclusion.

Great find on vectorized_filter @adehecq! I was curious why we missed out on it before, and the reason is because it's brand new, came out in SciPy 1.16.0 released end-of-July this summer: https://github.com/scipy/scipy/releases/tag/v1.16.0.

This is amazing news, it means we can have performant convolutions/filters with NaNs without requiring Numba, and comforts me into moving Numba into an optional dependency, while having SciPy as default main dependency (both for xDEM and GeoUtils).

@adebardo If the performance is indeed much better than generic_filter, tell us as we need to slightly update the SciPy engine for some terrain attributes in xDEM. 😉

Hello,
Thank you @adehecq for all this feedback, I’ll try to take it into account as quickly as possible.
The use of vectorized_filter is very interesting, but this version of SciPy is only compatible with Python 3.11. We need to discuss how to approach this issue.

The use of vectorized_filter is very interesting, but this version of SciPy is only compatible with Python 3.11. We need to discuss how to approach this issue.

Good catch! For new features, we can do this:

from packaging.version import Version

if Version(scipy.__version__) > Version("1.16.0"):
    generic_filter = scipy.ndimage.vectorized_filter
else:
    generic_filter = scipy.ndimage.generic_filter

That should solve it 😉 (i.e. it will work on any Python version)

Contexte

• Mooving filter.py from xDEM to Geouils to create a raster.filter() method
• Available filters : min/max, mean, median, gaussian, distance
• Liste des différentes actions
  • Remove "OpenCV" dependency
  • Optionnal use of numba to improve performance

Point de soucis

• Processing NaNs -> add _nan_safe_filter -> some % of errors (between 0.5% and 6%)

• _vectorized_filter -> python 3.11 is complicated