pass method instead of method as string

[haesleinhuepf]

Description

Hi Johannes @jo-mueller,

This is a resurrection of #14 and #19. Can we please work together on this a bit before merging it? It still has the bug you discovered in this discussion: #10 (comment) That's why this PR is a PR-draft. It is not ready yet for merging.

Thanks! :-)

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]

Thanks for starting this one. I think the easiest solution would be to move these to lines

        # Keep only ious above threshold
        img_sim[img_sim < threshold] = 0.0
        img_sim[img_sim < img_sim.max(axis=0)] = 0.0

into thresholded_intersection_over_union_matrix(...). It's not really a thresholded iou, but rather a non-maximum-suppressed iou, so I would suggest to rename the function to suppressed_intersection_over_union_matrix(...) or suppressed_jaccard_index_matrix(...), for that matter.

Edit: Actually, we could introduce a few different methods here already:

• intersection_over_union_matrix(...): No changes to iou before matching labels
• maximal_intersection_over_union_matrix(...): Keep only the maximal entries in each column
• suppressed_intersection_over_union_matrix(...): Suppress entries below suppression threshold

Depending on the matching-algorithm, one of these could be applicable.

[haesleinhuepf]

I think the easiest solution would be to move these to lines [...] It's not really a thresholded iou

Yes, great idea! I was also thinking of renaming the method. How about we call these things no longer something...matrix but rather a merger or something that decides what to merge.

I could then think of three implementations:

• merge_with_jaccard_above_threshold (that's what's currently implemented)
• merge_maximum_jaccard_if_above_threshold (I think that's what you're aiming for)
• merge_with_centroid_distance_below_threshold (that's just an alternative to demonstrate what else we could do with this, in case the API is designed properly.

[haesleinhuepf]

Lol. You're editing old posts while I'm typing new posts :-)

[jo-mueller]

Yes, great idea! I was also thinking of renaming the method. How about we call these things no longer something...matrix but rather a merger or something that decides what to merge.

Yes, ultimately it was also my idea to keep the metric and the matching separate. Depending on the metric we should define a few conventions, like:

• High value = high similarity --> euclidian distances between centroids would have to be inverted, otherwise large distance would be counted as similarity.
• Normalization between 0 and 1? Not sure if this makes sense/is necessary, though.
• Something I'm missing?

[jo-mueller]

It could also be an option to split the process into three steps:

• metric: IoU, euclidian distances of centroids, etc
• filter: keep only maximal entries/ suppress below threshold
• merge: used algorithm to match up labels based on filtered similarity matrix.

[haesleinhuepf]

It could also be an option to split the process into three steps:

Yes! Fantastic idea!

[jo-mueller]

I think it looks rather good so far :)

It could also be an option to split the process into three steps:

Yes! Fantastic idea!

I added this to the PR.

[haesleinhuepf]

Hey Johannes @jo-mueller ,

awesome! The new functionality looks great. We're almost there! Just some documentation issues to fix.

Let me know when I should take a look again!

[jo-mueller]

I added some more comments to each of the functions docstrings and rebuilt the docs. I think this can be merged.

[haesleinhuepf]

Hi Johannes @jo-mueller ,

just some more suggestions. I'm wondering why we need to correct variable names again and again. Furthermore, do these stars in *variable* mean something? I'd like to learn!

[jo-mueller]

Hi Robert @haesleinhuepf

Furthermore, do these stars in variable mean something?

They would make optional arguments appear in italics in the sphinx-built documentation - just as a nice touch, there's no deeper meaning.

I'm wondering why we need to correct variable names again and again.

I merged from main where the names had not been changed appropriately yet - sorry for the hickup. For the future, feel free to simply point the finger at such things and I'll fix it right away.

[jo-mueller]

@haesleinhuepf

I think this is now clear to be merged.

[haesleinhuepf]

Agreed @jo-mueller. Thanks for working on this! :-)