BUG: Fix greycoprops correlation always returning 1 #2532
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2532 +/- ##
==========================================
+ Coverage 90.53% 90.54% +<.01%
==========================================
Files 304 304
Lines 21706 21715 +9
Branches 1872 1872
==========================================
+ Hits 19652 19661 +9
Misses 1714 1714
Partials 340 340
Continue to review full report at Codecov.
|
| @@ -138,21 +138,21 @@ def test_contrast(self): | |||
| normed=True, symmetric=True) | |||
| result = np.round(result, 3) | |||
| contrast = greycoprops(result, 'contrast') | |||
| np.testing.assert_almost_equal(contrast[0, 0], 0.586) | |||
| np.testing.assert_almost_equal(round(contrast[0, 0], 3), 0.585) | |||
There was a problem hiding this comment.
assert_almost_equal should have a digits keyword, so that you don't have to use round
skimage/feature/texture.py
Outdated
| @@ -214,6 +214,12 @@ def greycoprops(P, prop='contrast'): | |||
| assert num_dist > 0 | |||
| assert num_angle > 0 | |||
|
|
|||
| # normalize each GLMC | |||
There was a problem hiding this comment.
Perhaps add a catch for normed=False, that mentions that it now always happens?
Also, add a test that would catch a previously failing case.
There was a problem hiding this comment.
By catch do you mean a warning?
evanlimanto
force-pushed
the
greycoprops-normalize
branch
3 times, most recently
from
acb995e
to
be3e56b
Compare
|
@stefanv I've updated the docstring in skimage/feature/texture.py |
| def test_non_normalized_glcm(self): | ||
| img = (np.random.random((100, 100)) * 8).astype(np.uint8) | ||
| p = greycomatrix(img, [1, 2, 4, 5], [0, 0.25, 1, 1.5], levels=8) | ||
| np.testing.assert_(np.min(greycoprops(p, 'correlation')) < 1.0) |
skimage/feature/texture.py
Outdated
| @@ -166,6 +167,7 @@ def greycoprops(P, prop='contrast'): | |||
| .. math:: \\sum_{i,j=0}^{levels-1} P_{i,j}\\left[\\frac{(i-\\mu_i) \\ | |||
| (j-\\mu_j)}{\\sqrt{(\\sigma_i^2)(\\sigma_j^2)}}\\right] | |||
|
|
|||
| Each GLCM is normalized to have a sum of 1 before calculation. | |||
There was a problem hiding this comment.
What does "before calculation" mean here?
There was a problem hiding this comment.
Updated comment to "before computation of texture properties".
evanlimanto
force-pushed
the
greycoprops-normalize
branch
from
783708f
to
daa831a
Compare
|
@stefanv Wouldn't it deserve a special note for the next release? Also, greycoprops and greycomatrix now have different default behviors. One is not normalized by default and the second is always normalized. |
|
This bugfix is pending for a while. Anyone to review @scikit-image/core |
|
@sciunto You are absolutely right! Thanks for the ping, thanks @evanlimanto for the fix, and thanks everyone else who contributed to the discussion in #2012! |
|
I'm currently working on some speedups for One possible solution would be to have an (optional) If this is not the appropriate place for this discussion, please let me know where I should take it. |
|
@MattWenham Thanks for keeping this up. Github is the right place. I would suggest to continue the discussion in #3413 for more visibility (even copy paste adapt your comment to #3413). |
Fixes issue #2012 by always normalizing the Gray-level Co-occurrence Matrices to a sum of 1.