Feature: Only detect features if all previous thresholds have been met

[lettlini]

• Have you followed our guidelines in CONTRIBUTING.md?
• Have you self-reviewed your code and corrected any misspellings?
• Have you written documentation that is easy to understand?
• Have you written descriptive commit messages?
• Have you added NumPy docstrings for newly added functions?
• Have you formatted your code using black?
• If you have introduced a new functionality, have you added adequate unit tests?
• Have all tests passed in your local clone?
• If you have introduced a new functionality, have you added an example notebook?
• Have you kept your pull request small and limited so that it is easy to review?
• Have the newest changes from this branch been merged?

This PR introduces the ability to only detect features if all previous thresholds have been met as discussed in #261.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.17 🎉

Comparison is base (90ba004) 54.60% compared to head (5a1678e) 54.77%.

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.5.0     #283      +/-   ##
=============================================
+ Coverage      54.60%   54.77%   +0.17%     
=============================================
  Files             15       15              
  Lines           3170     3182      +12     
=============================================
+ Hits            1731     1743      +12     
  Misses          1439     1439              

Flag	Coverage Δ
unittests	54.77% <100.00%> (+0.17%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tobac/feature_detection.py	85.75% <100.00%> (+0.44%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[w-k-jones]

@freemansw1 should we aim to include this in v1.5 so that it does not clash with the shift to xarray for v1.6?

[freemansw1]

@freemansw1 should we aim to include this in v1.5 so that it does not clash with the shift to xarray for v1.6?

If we can do it quickly. I am disinclined from adding more that we need to wait on. If we decide to wait on this, we can always include a minor feature like this in a v1.5.x release before v1.6.

[lettlini]

I think I will need some more time to polish this PR (e.g. add unit tests). Therefore, I would be in favor of including this in a minor release as @freemansw1 suggested.

[w-k-jones]

Had a quick look just now and it looks good, will have a closer look at the tests as requested. I did find a separate bug that is shown in the example notebook regarding previously detected features (#298), so that's a nice catch!

[w-k-jones]

@lettlini are you happy if I add the fix for #298 to this PR? I think it will require changing the order in which remove_parents and your additions are performed in, so I think it will be easier to avoid conflicts if I commit the changes to your branch

[lettlini]

@lettlini are you happy if I add the fix for #298 to this PR? I think it will require changing the order in which remove_parents and your additions are performed in, so I think it will be easier to avoid conflicts if I commit the changes to your branch

absolutely no problem, Thank you @w-k-jones

[w-k-jones]

Have created a PR for this change at lettlini#1 (comment)

[JuliaKukulies]

Nice job, @lettlini! I had a detailed look at both the unit test and the example notebook and I think they are both clear and make sense. So approved from my side.

Also, nice catch on the remove_parents bug @w-k-jones !

[freemansw1]

Just noting that we should hold off on merging this until #298 is resolved.

[w-k-jones]

Just noting that we should hold off on merging this until #298 is resolved.

Apologies, I should have been a bit more explicit. #298 is resolved within this PR, as I added it directly to Kolya's branch to avoid any conflicts (see lettlini#1)

[freemansw1]

Thanks @w-k-jones for the clarification. I'm happy for this to be merged, then!