Remove bug in ARE in evaluate.py #89

JohnnyTeutonic · 2017-03-22T10:43:46Z

As per discussion at cremi/cremi_python#3 (comment) about errors with the normalisation of the Adapted Rand Errror, I have incorporated the changes to the ARE, adapted from the fixed code found in the above discussion, which is seen in the link: https://gist.github.com/thouis/63888c375cbeb2f702e94e2e82eebee8.
The main change to the code is the removal of the division by 'n' , which previously had included division by both zero and non-zero pixels, which occurred when calculating the sum of the pixels in segments A and B. So now this code reflects only division by non-zero pixels, which is what should have been reflected in the reference implementation.

coveralls · 2017-03-22T10:51:21Z

Coverage decreased (-0.05%) to 50.079% when pulling 81c0f60 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-29T05:40:43Z

Coverage decreased (-0.03%) to 50.105% when pulling de738fc on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-29T05:43:24Z

Coverage decreased (-0.03%) to 50.105% when pulling de738fc on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-29T05:52:47Z

Coverage decreased (-0.03%) to 50.105% when pulling 685ac89 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-29T06:01:22Z

Coverage decreased (-0.08%) to 50.052% when pulling f760002 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-29T06:17:21Z

Coverage decreased (-0.08%) to 50.052% when pulling fe8468a on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-29T06:21:57Z

Coverage decreased (-0.08%) to 50.052% when pulling 7ca16be on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-29T06:34:21Z

Coverage decreased (-0.08%) to 50.052% when pulling 86e6dd8 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-03-30T09:34:30Z

Coverage decreased (-0.05%) to 50.079% when pulling 6c3b5a3 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-04-17T00:27:10Z

Coverage decreased (-0.01%) to 50.118% when pulling 8391091 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-04-21T06:40:45Z

Coverage increased (+0.5%) to 50.618% when pulling c7abd48 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

jni

@JohnnyTeutonic pretty good but still some work to do here! =)

jni · 2017-04-21T08:35:08Z

gala/evaluate.py

@@ -927,10 +927,11 @@ def rand_by_threshold(ucm, gt, npoints=None):
    return np.concatenate((ts[np.newaxis, :], result), axis=0)

 def adapted_rand_error(seg, gt, all_stats=False):
+    from gala import evaluate as ev


jni · 2017-04-21T08:35:27Z

gala/evaluate.py

@@ -942,6 +943,7 @@ def adapted_rand_error(seg, gt, all_stats=False):
    all_stats : boolean, optional
        whether to also return precision and recall as a 3-tuple with rand_error

+


Remove this extra newline

jni · 2017-04-21T08:35:34Z

gala/evaluate.py

@@ -952,40 +954,43 @@ def adapted_rand_error(seg, gt, all_stats=False):
    rec : float, optional
        The adapted Rand recall.  (Only returned when `all_stats` is ``True``.)

+


This one too

jni · 2017-04-21T08:37:01Z

gala/evaluate.py

+    # This is the contingency table obtained from segA and segB, we obtain
+    # the marginal probabilities from the table.
+    p_ij = ev.contingency_table(segA, segB, norm=False)
+    contingency_table = p_ij.A


WHOA. Do not do this. This instantiates a potentially enormous array. The contingency table should be usable in sparse format. .A is used to make numpy arrays out of e.g. the marginals, which are much smaller (n + m instead of n * m)

jni · 2017-04-21T08:38:05Z

gala/evaluate.py


-    p_ij = sparse.csr_matrix((ones_data, (segA[:], segB[:])), shape=(n_labels_A, n_labels_B))
+    # Sum of the joint distribution squared
+    sum_p_ij = np.sum(contingency_table**2)


sum_p_ij = np.sum(p_ij.data ** 2)

See Elegant SciPy, Nunez-Iglesias et al, Chapter 5. =P

actually, check the comment about sum_a: sum_p_ij = p_ij.data @ p_ij.data.

jni · 2017-04-21T08:44:54Z

gala/evaluate.py

-    precision = sumAB / sumB
-    recall = sumAB / sumA
+    precision = (sum_p_ij - n )/ (sum_a - n)
+    recall = (sum_p_ij -n )/ (sum_b - n)


PEP8 here also

jni · 2017-04-21T08:45:49Z

gala/evaluate.py


    fScore = 2.0 * precision * recall / (precision + recall)
-    are = 1.0 - fScore
-
+    are = abs(1.0 - fScore)


Just write 1 - f_score here — the 1.0 is a legacy construct from Python 2.

Also, you shouldn't need abs here...?

jni · 2017-04-21T08:46:26Z

gala/evaluate.py

-    precision = sumAB / sumB
-    recall = sumAB / sumA
+    precision = (sum_p_ij - n )/ (sum_a - n)
+    recall = (sum_p_ij -n )/ (sum_b - n)

    fScore = 2.0 * precision * recall / (precision + recall)


Not your responsibility, but you might as well fix fScore -> f_score while you're here. =)

jni · 2017-04-21T08:47:31Z

tests/test_evaluate.py

+def test_are():
+    seg = np.array([[0,1], [1,0]])
+    gt = np.array([[1,2],[0,1]])
+    assert_almost_equal(abs(ev.adapted_rand_error(seg,gt)),0.3333333)


Why is there an abs here?

jni · 2017-04-21T08:47:45Z

tests/test_evaluate.py

+    seg = np.array([[0,1], [1,0]])
+    gt = np.array([[1,2],[0,1]])
+    assert_almost_equal(abs(ev.adapted_rand_error(seg,gt)),0.3333333)
+    assert seg.shape == gt.shape


This is always true by construction, so you can remove it.

coveralls · 2017-04-22T00:47:48Z

Coverage increased (+0.5%) to 50.618% when pulling 1c612af on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

coveralls · 2017-04-22T01:15:04Z

Coverage increased (+0.5%) to 50.592% when pulling 9018d0c on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

JohnnyTeutonic · 2017-04-22T01:19:47Z

Okay, I included all your changes and also removed the unnecessary ravel functional calls (segA = np.ravel(gt), segB = np.ravel(seg)) at the beginning of the function, as I realised that the contingency table already flattens the input arrays anyway. So that removes some more redundant code.

jni

@JohnnyTeutonic I've requested one more change. It's minor, I promise! =)

jni · 2017-05-02T11:33:49Z

tests/test_evaluate.py

+
+def test_are():
+    seg = np.array([[0, 1], [1, 0]])
+    gt = np.array([[1, 2],[0, 1]])


@JohnnyTeutonic part of the reason this whole issue arose was the way 0 was being treated as background. We've decided to not do this, but, in case we eventually do, it would be best if this test didn't use the 0 label. So can you please relabel 0s to 3 in both volumes?

coveralls · 2017-05-02T23:09:48Z

Coverage increased (+0.5%) to 50.618% when pulling 90988b6 on JohnnyTeutonic:adapted_rand_index into 6613db2 on janelia-flyem:master.

JohnnyTeutonic added 12 commits March 22, 2017 20:59

Include mask to foreground in 'A'

27f29ab

Include parameter of datatype 'uint64' into 'p_ij'

bc7034a

Include comment about use of background pixels in Segb

5d4df1f

Include variables to hold pixels from segment B and sum of pixels

3a2074e

Include comment about new code removing division by 'n'

2577399

Include variable holding sum of joint distribution of elements of B

8338af8

Include marginal probabilities

9f10602

remove variables 'a,b,c,'d' and old version of 'a_i' and 'b_i'

a16917e

Include 'sum_a' and 'sum_b'

1e8afb7

Comment out old code no longer required for new version of ARE

6ade0ab

Comment out old versions of 'precision' and 'recall'

c00ed75

Include new versions of 'precision' and 'recall'

81c0f60

JohnnyTeutonic added 2 commits March 29, 2017 16:34

Include optional formal parameter 'count_zeros'

76f3995

Include comments about the inclusion of 'count_zeros' parameter

de738fc

JohnnyTeutonic added 2 commits March 29, 2017 16:48

Change default 'count_zeros' to True

685ac89

Include conditional if statement for 'count_zeros==False'

f760002

JohnnyTeutonic added 2 commits March 29, 2017 17:12

Include comment about formal parameter 'count_zeros'

fe8468a

Include comments outside of docstring about use of non-zeros

7ca16be

Include tidying up of comments and punctuation

86e6dd8

JohnnyTeutonic added 3 commits March 30, 2017 20:11

Clarify comments about use of non-zeros parameter

83852b7

Remove commented old code

5af7511

Add basic test to assert same shape of seg and gt

6c3b5a3

Subtract 'n' from precision and recall parameters

8391091

JohnnyTeutonic added 5 commits April 19, 2017 16:41

Add in test for ARE

6e80bce

Change ARE to include contingency table

27852b2

Include import of assert_almost_equal

1805312

Include test of absolute value of ARE

108737f

Change test_evalute and ARE in evaluate

c7abd48

jni requested changes Apr 21, 2017

View reviewed changes

Conform the test to Pep8 and remove abs function

1c612af

Include optimisations to 'ARE' and conform to pep8

9018d0c

jni approved these changes May 2, 2017

View reviewed changes

jni reviewed May 2, 2017

View reviewed changes

Change test_are to remove zeros in array

90988b6

jni merged commit f84a682 into janelia-flyem:master May 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove bug in ARE in evaluate.py #89

Remove bug in ARE in evaluate.py #89

JohnnyTeutonic commented Mar 22, 2017

coveralls commented Mar 22, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 30, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 21, 2017

jni left a comment

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

jni Apr 21, 2017

coveralls commented Apr 22, 2017

coveralls commented Apr 22, 2017

JohnnyTeutonic commented Apr 22, 2017

jni left a comment

jni May 2, 2017

coveralls commented May 2, 2017

		@@ -942,6 +943,7 @@ def adapted_rand_error(seg, gt, all_stats=False):
		all_stats : boolean, optional
		whether to also return precision and recall as a 3-tuple with rand_error

		@@ -952,40 +954,43 @@ def adapted_rand_error(seg, gt, all_stats=False):
		rec : float, optional
		The adapted Rand recall. (Only returned when `all_stats` is ``True``.)

Remove bug in ARE in evaluate.py #89

Remove bug in ARE in evaluate.py #89

Conversation

JohnnyTeutonic commented Mar 22, 2017

coveralls commented Mar 22, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 30, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 21, 2017

jni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 22, 2017

coveralls commented Apr 22, 2017

JohnnyTeutonic commented Apr 22, 2017

jni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 2, 2017