-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix minimum distance filtering for varying vertical coordinates #452
Conversation
…during feature detection
@freemansw1 are you happy to review this? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.x #452 +/- ##
=============================================
+ Coverage 60.81% 61.14% +0.32%
=============================================
Files 23 23
Lines 3527 3580 +53
=============================================
+ Hits 2145 2189 +44
- Misses 1382 1391 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Linting results by Pylint:Your code has been rated at 8.74/10 (previous run: 8.74/10, +0.00) |
…into min_distance_3D_fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @w-k-jones . Checks out on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good @w-k-jones! Thanks for also fixing the statistics issue. Just one thing I am wondering with regard to #449
I have re-added the unsmoothed statistics functionally which was removed by mistake. The remaining failing tests and the formatting and one of the matrix tests which has the sporadic spectral filtering test bug. @JuliaKukulies & @freemansw1 are you happy for me to merge? |
Yes, ready to merge from my side! Thanks @w-k-jones ! |
Resolves #430 and #413
The order of coordinate interpolation and minimum distance filtering has been swapped, so that the z coordinate can be used for finding the spacing of features rather than assuming a fixed vertical grid spacing. The
filter_min_distance
has also been fixed so that the provided z coordinate will be used, rather than always usingdz
. Incidentally, another bug was fixed infeature_detection_multithreshold_timestep
was fixed, in which statistics were being recalculated at each threshold value, rather than once at the end.Ideally, I would like to move adding coordinates and distance filtering inside
feature_detection_multithreshold_timestep
, but at the moment we are not passing through the required info to do this. As the feature detection code is in need of refactoring, I'll leave this until after v1.6 to avoid conflicts with the switch to xarray