-
Notifications
You must be signed in to change notification settings - Fork 54
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
Update references and documentation on threshold use #365
Update references and documentation on threshold use #365
Conversation
… values in feature detection and segmentation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.x #365 +/- ##
=============================================
+ Coverage 56.68% 56.70% +0.02%
=============================================
Files 19 19
Lines 3426 3428 +2
=============================================
+ Hits 1942 1944 +2
Misses 1484 1484
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looks good, @JuliaKukulies, just one minor thought
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.
Ok great, approving this now @JuliaKukulies.
If we wanted to include the option to be inclusive with the threshold for segmentation, would we also include the option to be exclusive for feature detection? Might otherwise cause confusion if an option isn't available in both functions.
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.
All looks good to me - documentation on thresholds is now much clearer :)
Hm yes, good point. However, I find it also confusing when the options exist for both but in different directions. I feel that this needs some further discussion so I will not include it in this PR/release. Are you OK with that @freemansw1 @w-k-jones ? |
Thanks for reviewing this @snilsn and @harrietgilmour ! |
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.
I have a few minor comments, but am overall very happy! Thanks for doing this!
I am merging this now and creating an issue for the future implementation of optional treatment of threshold values. |
Addresses #346 and #347.
Note that for #346, I have only changed the documentation now, but could also update the segmentation function with an optional input parameter to make the threshold inclusive. Thoughts?
Also, happy to get help on recent publications that I have missed. @freemansw1 maybe you are aware of additional ones for INCUS or from any CSU students that I could not find :)