Variable names

[haesleinhuepf]

Description

Hi Johannes @jo-mueller ,

I just changed the variable names as I suggested earlier in #2. Could you please find out how you can rename variables in a way in your IDE avoiding that parameters have different names in the parameter list of a function compared to the documentation of the function? In pycharm this works using Shift+F6:

Type of change

• Bug-fix
• New feature
• Breaking change
• Documentation update

References

Tests

• I adapted existing tests, because
• I added new tests to cover the code change
• All tests pass with my change

Final checks

• My change is the minimal possible work for the desired feature/fix
• I updated the documentation where necessary to cover the change

[jo-mueller]

Looks good to me. I'm wondering if we could change the function name from intersection_over_union_matrix to something like iou_matrix? It's just a personal preference for short function names, but not a must.

[haesleinhuepf]

Looks good to me. I'm wondering if we could change the function name from intersection_over_union_matrix to something like iou_matrix? It's just a personal preference for short function names, but not a must.

How about a jaccard_index_matrix? That's how it's called in clij.

[jo-mueller]

@haesleinhuepf

Looks good to me. I'm wondering if we could change the function name from intersection_over_union_matrix to something like iou_matrix? It's just a personal preference for short function names, but not a must.

How about a jaccard_index_matrix? That's how it's called in clij.

Sounds good to me, but I think I'll do this in a separate PR to main, since intersection_over_union_matrix is already called in some tests in the main branch that are not referenced on this branch.

Regarding the new test function:

def test_match_labels2():
    
    labels_x = np.asarray([1, 1, 1, 0, 0, 2, 2, 4, 4, 4, 0], dtype=np.uint8)
    labels_y = np.asarray([1, 1, 2, 0, 0, 3, 3, 4, 4, 4, 5], dtype=np.uint8)

    reference_y_matched = np.asarray([1, 1, 1, 0, 0, 2, 2, 4, 4, 4, 5], dtype=np.uint8)

    labels_y_matched = biau.label.match_labels(labels_x, labels_y)

    assert np.array_equal(reference_y_matched, labels_y_matched)

I think we should discuss our preferred format. Right now, thresholded_intersection_over_union_matrix returns a boolean array in this branch. This leads to a behaviour in match_labels() which allows multiple labels from slice z+1 to be merged with labels from slice z whenever the overlap is larger than 0.3 (default threshold). I could imagine that this may lead to problems downstream, because a label can have an overlap of 0.3 with up to three labels in the adjacent slice.

The original behavior from the cellpose repository is a little different: The IoU matrix is not binarized, it is only thresholded in a sense that entries in the IoU matrix smaller than x are set to zero:

        IoU[IoU< threshold] = 0.0  # suppress small overlaps
        IoU[IoU< IoU.max(axis=0)] = 0.0  # suppress non-maximum overlaps

Other entries are unaffected from this so that only labels with the largest overlap will be merged.

The difference is very subtle but can lead to very different results, as the above test shows. In the case of a binary IoU matrix the correct result is this:

reference_y_matched = np.asarray([1, 1, 1, 0, 0, 2, 2, 4, 4, 4, 5], dtype=np.uint8)

In the case of a maximal IoU approach with suppressed entries, the correct result is this:

reference_y_matched = np.asarray([1, 1, 6, 0, 0, 2, 2, 4, 4, 4, 5], dtype=np.uint8)

[haesleinhuepf]

I think we should discuss our preferred format. Right now, thresholded_intersection_over_union_matrix returns a boolean array in this branch. This leads to a behaviour in match_labels() which allows multiple labels from slice z+1 to be merged with labels from slice z whenever the overlap is larger than 0.3 (default threshold). I could imagine that this may lead to problems downstream, because a label can have an overlap of 0.3 with up to three labels in the adjacent slice.

Oh yes, good point! Actually, we should have discssed that in #14
before it was merged, because that's what this PR was about. Let's chat about that on Wednesday after the group meeting

[haesleinhuepf]

I will try do undo the merge of #14 in the meantime...

[haesleinhuepf]

Ok @jo-mueller , please check this PR again. It now only contains variable name changes. Looking foward to hear what you think!

[jo-mueller]

Looks good to me, feel free to merge 👍

[jo-mueller]

Small hold-up: Removing #14 from this also reverted the location of the tests to /label/tests/... instead of tests/...

I'll fix this

[haesleinhuepf]

I'll fix this

You can also merge this PR as you're assigned for this anyway. Thanks! 🙏