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.

Sign up for GitHub

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

Jump to bottom

Create own python class for every model for action cls, det, and classification tasks #2776

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Create own python class for every model for action cls, det, and classification tasks #2776

merged 8 commits into from
Jan 16, 2024

Conversation

vinnamkim
Copy link
Contributor

Summary

  • Same as title. Other tasks will be resolved in the following PR.

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added e2e tests for validation.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache 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) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@vinnamkim
Add export() and register_explain_hook() to OTXModel
9d5818a 
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@vinnamkim
Add num_classes to OTXModel and revisit DataMetaInfo
bc6e7fc 
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@vinnamkim
Fix test error
3e4e7df 
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@vinnamkim
Change action cls and det
1afea45 
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@github-actions github-actions bot added TEST Any changes in tests OTX 2.0 labels Jan 11, 2024
@vinnamkim
Change classification models
03345ba 
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@vinnamkim
Merge remote-tracking branch 'upstream/v2' into create-own-python-cla…
09e274a 
…ss-for-every-model-2

Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@vinnamkim vinnamkim force-pushed the create-own-python-class-for-every-model-2 branch from 8db6f45 to 9dfaa94 Compare January 11, 2024 10:54
@vinnamkim vinnamkim marked this pull request as ready for review January 11, 2024 10:58
@vinnamkim
Fix light=False as default
9dfaa94 
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
samet-akcay
samet-akcay requested changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@samet-akcay samet-akcay left a comment

Choose a reason for hiding this comment

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

thanks for the effort!

Overall, it is a good step towards having an api-level definition for the models. However, this is still heavily rely on an external configuration file, which is hard to modify from API.

In addition to this, I've got some minor comments.

As we move on, I think we should make this more API friendly by avoiding config dependency

src/otx/algo/action_classification/__init__.py Outdated Show resolved Hide resolved
src/otx/algo/action_classification/x3d.py Show resolved Hide resolved
src/otx/algo/action_detection/x3d_fastrcnn.py Show resolved Hide resolved
src/otx/algo/utils/mmconfig.py Outdated Show resolved Hide resolved
src/otx/algo/utils/mmconfig.py Outdated Show resolved Hide resolved
src/otx/algo/utils/mmconfig.py Outdated Show resolved Hide resolved
@vinnamkim
Make read_mmconfig() more descriptive
669d935 
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
This was referenced Jan 12, 2024
Merged
Merged
Merged
@vinnamkim vinnamkim requested a review from samet-akcay January 12, 2024 10:21
sungmanc
sungmanc approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@sungmanc sungmanc left a comment

Choose a reason for hiding this comment

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

Thanks for the change. LGTM

@vinnamkim vinnamkim enabled auto-merge (squash) January 16, 2024 10:02
@vinnamkim
Copy link
Contributor Author

@samet-akcay
Do you have any concern more?

@samet-akcay
Copy link
Contributor

@samet-akcay Do you have any concern more?

The remaining comments are for future reference.

Sorry, forgot to approve this one :)

@vinnamkim vinnamkim merged commit cc0c49d into openvinotoolkit:v2 Jan 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sovrasov sovrasov sovrasov left review comments

@jaegukhyun jaegukhyun jaegukhyun approved these changes

@sungmanc sungmanc sungmanc approved these changes

@harimkang harimkang harimkang approved these changes

@samet-akcay samet-akcay samet-akcay approved these changes

@eugene123tw eugene123tw Awaiting requested review from eugene123tw eugene123tw is a code owner

@kprokofi kprokofi Awaiting requested review from kprokofi kprokofi is a code owner

@chuneuny-emily chuneuny-emily Awaiting requested review from chuneuny-emily chuneuny-emily is a code owner

Assignees
No one assigned
Labels
TEST Any changes in tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vinnamkim @samet-akcay @sovrasov @harimkang @sungmanc @jaegukhyun