-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Threshold generalized histogram #5332
base: main
Are you sure you want to change the base?
Threshold generalized histogram #5332
Conversation
…compatible for different connectivity
…compatible for different connectivity
…compatible for different connectivity : adding test case
…nto threshold_generalized_histogram
Hello @divyank0! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-05-01 15:23:54 UTC |
counts, bin_centers = histogram(image.ravel(), 256, source_range="image") | ||
|
||
# (nu, tau, kappa, omega, threshold) | ||
possible_values = [ |
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 guess you want to parametrize this test 🙂
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.
done
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.
Thank you for your contribution! I just took a quick look at your code. I will review more thoroughly later.
skimage/filters/thresholding.py
Outdated
default_nu, default_tau, default_kappa, default_omega) | ||
>>> binary = data<=t | ||
""" | ||
assert nu >= 0 |
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.
Use assertions to self-test the program, not to handle wrong user input. Instead, raise an exception and print an error message. Possibly handle the exception by overwriting wrong input values with the respective default parameter values?
I think we need to discuss why this addition is valuable for the library. A broad range of threshold algorithm exists and we need to focus on the most famous one. The proposed one seems to be new. Is there a clear advantage? |
Sure Francois, there is some initial Description written by Juan on the importance of this Feature. Also Following points I found useful:
Thresholds in image 2 to 6 are calculated using GHT with different hyperparameters, 7th is Otsu's Implementation available in scikit-image. My knowledge in this subject is limited as compared to others, Hence I welcome any and all comments. |
@sciunto see #5189 for the motivation. The fact that a number of existing algorithms can be generalised to this method, and that this method when properly tuned can outperform many neural nets, is a clear advantage. Having said this, @divyank0 the main issue I see is that GHT Otsu and existing Otsu should match. If they don't, that suggests that there is a bug in the implementation somewhere. Also, see my comment on the original issue for my thoughts about the API:
|
any update ? @jni |
Description
#5189
Feature request: generalized_histogram_thresholding algorithms
It contains the required function, doc string and test cases.
I am working on Gallery examples. Would love a Review till here.
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.