Skip to content
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

Implement gale shapley algorithm for label-matching #24

Merged
merged 34 commits into from
Dec 15, 2021

Conversation

jo-mueller
Copy link
Contributor

Description

The preiously implemented strategy for label-matching based on picking the label with the respective highest intersection over union did not perform so nicely for complicated arrangements of overlapping labels. I implemented the Gale-Shapley algorithm to deal with such situations and I think it performs reasonably fast and well.

The previous version also removed the entries in the similarity matrix referring to the background. For the implementation of above algorithm it was easier to force-match the background in both images with wach other (setting similarity_matrix[0,0]=1.0 instead of performing matching operations on similarity_matrix[1:, 1:]. This required minor changes in the previous implementation of max_similarity(...) matching.

Note: The current implementation is rather long in terms of code - if you have suggestions for better implementations, let me know, I'll make this a draft in the meantime.

@zoccoler, feel free to have a look at the new code as well :)

Type of change

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

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 jo-mueller added the enhancement New feature or request label Nov 15, 2021
@jo-mueller jo-mueller self-assigned this Nov 15, 2021
@jo-mueller jo-mueller marked this pull request as draft November 15, 2021 21:46
Copy link
Member

@haesleinhuepf haesleinhuepf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Johannes @jo-mueller ,

wow, cool stuff you're adding here. I have mostly questions about documentation.

Thanks!

Robert

biapol_utilities/label/_match_labels.py Outdated Show resolved Hide resolved
biapol_utilities/label/_matching_algorithms.py Outdated Show resolved Hide resolved
biapol_utilities/label/_matching_algorithms.py Outdated Show resolved Hide resolved
biapol_utilities/label/_matching_algorithms.py Outdated Show resolved Hide resolved
tests/test_match_labels.py Outdated Show resolved Hide resolved
tests/test_match_labels.py Outdated Show resolved Hide resolved
biapol_utilities/label/_filter_similarity_matrix.py Outdated Show resolved Hide resolved
biapol_utilities/label/_filter_similarity_matrix.py Outdated Show resolved Hide resolved
biapol_utilities/label/_matching_algorithms.py Outdated Show resolved Hide resolved
@jo-mueller jo-mueller marked this pull request as ready for review November 30, 2021 18:02
@jo-mueller
Copy link
Contributor Author

@haesleinhuepf I think tests should now cover the new algorithm and the docstrings are in order for good documentation. Feel free to merge if you think it's fit.

@haesleinhuepf
Copy link
Member

Hi @jo-mueller we have now conflicts with the documentation. Could you try to fix it and/or tell me how to do this?

@jo-mueller
Copy link
Contributor Author

Hey @haesleinhuepf ,

in this case the conflicts came from rather minor differences in the docstrings of some functions. I think the easiest approach to address these conflicts is the following:

  • Resolve conflicts in script files (or generally, non-documentation files)
  • For all other files, taking either version should be fine.
  • Rebuild docs - this should change the documentation files to a state that allows merging without conflicts.

I fixed it for this case.

@jo-mueller
Copy link
Contributor Author

jo-mueller commented Dec 3, 2021

I fixed it for this case.

For future reference: I think this is also a good reason why it could make sense to keep the documentation on a separate branch (github offers to default to gh-pages) through a github action such as this or this. Thus, we would only make sure to resolve conflicts between scripts on our branches and the documentation would be automatically synchronized.

@haesleinhuepf
Copy link
Member

Hi Johannes @jo-mueller ,

awesome! I just created issue #29 so that we keep track of the documentation issue. Otherwise, I would merge. Do you think it's ready?

@jo-mueller
Copy link
Contributor Author

Yes :)

Co-authored-by: Robert Haase <haesleinhuepf@users.noreply.github.com>
@haesleinhuepf haesleinhuepf merged commit 292d353 into main Dec 15, 2021
@haesleinhuepf
Copy link
Member

Merged. Thanks for sharing this amazing algorithm Johannes @jo-mueller !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants