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

Hierarchical Labeling #742

Merged

Conversation

bonhunko
Copy link
Contributor

@bonhunko bonhunko commented Oct 17, 2022

Summary

Resolves 93482

  • Add LabelGroup option to datumaro for representing 'exclusive'

How to test

  1. Generate a dataset containing hierarchical labels.
  2. Export the dataset in 'datumaro' format.
  3. The labels which disobey the single-selection rule will be filtered-out in the exported dataset

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bonhunko bonhunko changed the title Feature/hierarchical labeling [WIP] Hierarchical Labeling Oct 17, 2022
@bonhunko bonhunko force-pushed the feature/hierarchical_labeling branch 6 times, most recently from 7bec117 to 4ca506e Compare October 17, 2022 22:43
@bonhunko bonhunko marked this pull request as ready for review October 17, 2022 22:49
@bonhunko bonhunko changed the title [WIP] Hierarchical Labeling Hierarchical Labeling Oct 17, 2022
@bonhunko bonhunko force-pushed the feature/hierarchical_labeling branch 7 times, most recently from f3287d4 to 192820f Compare October 18, 2022 00:50
tests/test_labeling.py Outdated Show resolved Hide resolved
tests/test_labeling.py Outdated Show resolved Hide resolved
datumaro/plugins/datumaro_format/converter.py Outdated Show resolved Hide resolved
datumaro/plugins/datumaro_format/converter.py Outdated Show resolved Hide resolved
datumaro/plugins/datumaro_format/converter.py Outdated Show resolved Hide resolved
vinnamkim
vinnamkim previously approved these changes Oct 18, 2022
Copy link
Contributor

@vinnamkim vinnamkim left a comment

Choose a reason for hiding this comment

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

LGTM.
But I don't still get the concept of this work. Our current data structure of categories has a parent entity. It means that it can represent any tree structure with arbitrary depth and width.

How about this case?

        l1(T)
      /        \
 l2(F)        l3(F)
  /  \         /    \
l4    l5     l6     l7

and Item1.annotations := [l1, l2, l3, l4, l5, l6, l7] (if label has single_selection=True, then it denotes label(T), otherwise, label(F)). Then, this filtering will make Item1.annotations = [l1, l2, l4, l5, l6, l7].

I feel this is a bit inconsistent because l6 and l7 is still alive although their parent l3 is filtered out.

wonjuleee
wonjuleee previously approved these changes Oct 18, 2022
Copy link
Contributor

@wonjuleee wonjuleee left a comment

Choose a reason for hiding this comment

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

LGTM

@wonjuleee wonjuleee dismissed stale reviews from vinnamkim and themself via 9d29c7b October 18, 2022 23:02
Copy link
Contributor

@wonjuleee wonjuleee left a comment

Choose a reason for hiding this comment

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

Could you check whether Vinnam's concern is true or not?

@bonhunko
Copy link
Contributor Author

Good, questions. This is a kind of architecture-level question. But I think that datumaro is needed to exports a general output. The detailed artchitecture-level filtering should be done by customer side. Because the detailed policy about how to handle hierarchical labeling should be determined by the customer. So please think that this implement only filter-out the error-cases that obviously wrong.

After facing some practical cases, we may change this filtering policy again.

@bonhunko bonhunko changed the title Hierarchical Labeling [WIP] Hierarchical Labeling Oct 19, 2022
@bonhunko
Copy link
Contributor Author

As vinnam suggested and discussed with wonju, I'll follow the LabelGroup implementation to mark the 'exclusive' after that I'll request review again.

@bonhunko bonhunko force-pushed the feature/hierarchical_labeling branch 4 times, most recently from 516e0c7 to 8eb3708 Compare October 19, 2022 09:51
@bonhunko bonhunko changed the title [WIP] Hierarchical Labeling Hierarchical Labeling Oct 19, 2022
@bonhunko
Copy link
Contributor Author

As we discussed, I import the LabelGroup format from the OTX, please review again.

@bonhunko bonhunko force-pushed the feature/hierarchical_labeling branch 4 times, most recently from def8347 to acea3b6 Compare October 19, 2022 15:06
tests/assets/datumaro_dataset/annotations/test.json Outdated Show resolved Hide resolved
tests/test_labeling.py Outdated Show resolved Hide resolved
vinnamkim
vinnamkim previously approved these changes Oct 20, 2022
Copy link
Contributor

@vinnamkim vinnamkim left a comment

Choose a reason for hiding this comment

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

LGTM.

@wonjuleee wonjuleee merged commit a8f3cc9 into openvinotoolkit:develop Oct 31, 2022
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.

3 participants