Skip to content
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

Add option to define different n_min_threshold dependent on thresholds #208

Conversation

JuliaKukulies
Copy link
Member

@JuliaKukulies JuliaKukulies commented Dec 1, 2022

This PR solves #18 by allowing users to input n_min_threshold (the minimum number of grid cells that must be above/below the set threshold(s)) as dict or list. That way, different n_min_threshold values are used for different threshold values in the feature detection.

The code changes encompass:

  • Changes to the function feature_detection_multithreshold_timestep() because here, individual threshold values are selected and passed to feature_detection_threshold()
  • Input validation to make sure that each threshold is assigned to one n_min_threshold. This means that the number of elements of the given dict or list has to be the same as the number of given thresholds. It is also made sure that these values are sorted accordingly when threshold values are sorted. When n_min_threshold is given as dict, the keys must correspond to the threshold values. When n_min_threshold is given as list, it is assumed that each n_min_threshold belongs to the value in threshold at the same position.
  • Additional test parameters in test_feature_detection.py to test the new feature
  • Modifications to the existing notebook on the input parameter n_min_threshold to show an example on our RTD page: see Different n_min_threshold for different threshold valuesunder Feature Detection Parameter Examples https://tobac--208.org.readthedocs.build/en/208/
  • 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?

@JuliaKukulies JuliaKukulies added the enhancement Addition of new features, or improved functionality of existing features label Dec 1, 2022
@JuliaKukulies JuliaKukulies added this to the Version 1.5 milestone Dec 1, 2022
@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Base: 41.01% // Head: 41.24% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (1040225) compared to base (aded93d).
Patch coverage: 78.57% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##           RC_v1.5.0     #208      +/-   ##
=============================================
+ Coverage      41.01%   41.24%   +0.22%     
=============================================
  Files             14       14              
  Lines           2321     2335      +14     
=============================================
+ Hits             952      963      +11     
- Misses          1369     1372       +3     
Flag Coverage Δ
unittests 41.24% <78.57%> (+0.22%) ⬆️

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

Impacted Files Coverage Δ
tobac/feature_detection.py 80.61% <78.57%> (-0.16%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Copy link
Collaborator

@snilsn snilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good work @JuliaKukulies, a great addition. I found one issue with sorting for the dictionary case and have a suggestion for that.

tobac/feature_detection.py Outdated Show resolved Hide resolved
tobac/feature_detection.py Outdated Show resolved Hide resolved
tobac/tests/test_feature_detection.py Outdated Show resolved Hide resolved
Juli and others added 4 commits December 8, 2022 21:12
@JuliaKukulies JuliaKukulies requested a review from snilsn December 8, 2022 20:30
Copy link
Collaborator

@snilsn snilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank fro these little changes @JuliaKukulies , I'm approving this.

Copy link
Member

@freemansw1 freemansw1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JuliaKukulies! I'm happy with this as-is. Feel free to merge when you're ready.

@JuliaKukulies
Copy link
Member Author

Thanks @freemansw1 and @snilsn for reviewing this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Addition of new features, or improved functionality of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants