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

Add tests #13

Merged
merged 7 commits into from
Nov 5, 2021
Merged

Add tests #13

merged 7 commits into from
Nov 5, 2021

Conversation

haesleinhuepf
Copy link
Member

Description

This is just adding some more tests. I would like to make sure the functions do what they are supposed to do

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

@haesleinhuepf haesleinhuepf changed the base branch from main to move-tests November 4, 2021 16:49
@haesleinhuepf haesleinhuepf changed the base branch from move-tests to move_tests November 4, 2021 16:49
@haesleinhuepf haesleinhuepf changed the title Add tests2 Add tests Nov 4, 2021
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, 0], dtype=np.uint8)

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

Choose a reason for hiding this comment

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

Hi Johannes @jo-mueller

could you please take a look at that test? I think this should fail. I think, the "5" should be a "1" in the reference. What do you think?

Copy link
Contributor

@jo-mueller jo-mueller Nov 5, 2021

Choose a reason for hiding this comment

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

Yes, reference_y_matched should essentially be a copy of labels_x here.

The "5" is actually correct and this is a good test because it covers a common situation: A label in slice_x may have been classified as two different labels in slice_y. match_labels is only allowed to merge labels in subsequent slices (x & y) if they have a large intersection, but it is not allowed to merge two labels in the same plane (i.e., x & x).

Thus, the labels with the largest overlap with 1 in labels_y will be matched (although they are already "1"), but the label that also overlaps the "1" in labels_x (i.e., label "2" in labels_y) will be labelled as a new label in the matched output since the label "2" is already occupied in labels_x.

This could be improved in future versions, but will require a much more elaborate approach than just measuring IoU.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, no I got it! Thanks :-)

Base automatically changed from move_tests to main November 5, 2021 10:38
@jo-mueller jo-mueller self-requested a review November 5, 2021 11:00
@jo-mueller jo-mueller marked this pull request as draft November 5, 2021 11:01
@jo-mueller
Copy link
Contributor

jo-mueller commented Nov 5, 2021

I added another test script for /label/_label_overlap_matrix.py - could you also have a look at it? @haesleinhuepf

@jo-mueller jo-mueller marked this pull request as ready for review November 5, 2021 11:27
@haesleinhuepf
Copy link
Member Author

Great! That's a good additional test.

@haesleinhuepf haesleinhuepf merged commit f6a7a6a into main Nov 5, 2021
@haesleinhuepf haesleinhuepf deleted the add_tests2 branch November 5, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants