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

Migrate default detection model to OTXv2 #2598

Merged
merged 25 commits into from
Nov 16, 2023
Merged

Migrate default detection model to OTXv2 #2598

merged 25 commits into from
Nov 16, 2023

Conversation

jaegukhyun
Copy link
Contributor

Summary

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

@github-actions github-actions bot added DEPENDENCY Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM TEST Any changes in tests BUILD labels Nov 3, 2023
@harimkang harimkang added the OTX 2.0 For OTX v2.0 label Nov 3, 2023
@jaegukhyun jaegukhyun force-pushed the v2-mmdet3.0 branch 4 times, most recently from 94b06fa to b506f80 Compare November 9, 2023 04:30
@jaegukhyun jaegukhyun marked this pull request as ready for review November 9, 2023 04:51
sungmanc
sungmanc reviewed Nov 9, 2023
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 huge effort, I left some comments. To be honest, I didn't review the modules folder since they came from the previous implementation.

src/otx/v2/adapters/torch/mmengine/dataset.py Outdated Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/engine.py Outdated Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/dataset.py Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/model.py Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/modules/datasets/dataset.py Show resolved Hide resolved
tests/v2/integration/cli/test_cli.py Show resolved Hide resolved
sungmanc
sungmanc approved these changes Nov 9, 2023
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.

LGTM

@jaegukhyun jaegukhyun changed the title V2 mmdet3.0 Migrate default detection model to OTXv2 Nov 10, 2023
goodsong81
goodsong81 requested changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@goodsong81 goodsong81 left a comment •
edited

Choose a reason for hiding this comment

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

Thank you for the work. You can ignore some design-related comment in this phase, but please keep the list for the future work.
A few questions and very minor change requests are following. You can postpone these fixes, too (according to your discretion).

By the way, why don't you rebase this PR after #2603 is merged?

src/otx/v2/adapters/datumaro/pipelines/load_image_from_otx_dataset.py Outdated Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/engine.py Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/engine.py Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/engine.py Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/modules/datasets/__init__.py Outdated Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/modules/evaluation/__init__.py Outdated Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/mmdet/modules/evaluation/evaluator.py Outdated Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/modules/hooks/adaptive_training_hook.py Outdated Show resolved Hide resolved
src/otx/v2/adapters/torch/mmengine/modules/hooks/adaptive_training_hook.py Show resolved Hide resolved
src/otx/v2/configs/detection/otx_mmdet_default.yaml Show resolved Hide resolved
goodsong81
goodsong81 approved these changes Nov 10, 2023
View reviewed changes
Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

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

Thanks!

@jaegukhyun
Copy link
Contributor Author

@wonjuleee I reflected your review by bb760eb.
Could you review once again?

eugene123tw
eugene123tw approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@eugene123tw eugene123tw left a comment

Choose a reason for hiding this comment

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

Looks good to me. Left some comments for future reference.

@wonjuleee
Copy link
Contributor

@wonjuleee I reflected your review by bb760eb. Could you review once again?

Thanks. Looks good to me.

samet-akcay
samet-akcay approved these changes Nov 16, 2023
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 @jaegukhyun, looking good!

@jaegukhyun jaegukhyun merged commit 39ab6c6 into openvinotoolkit:feature/v2 Nov 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goodsong81 goodsong81 goodsong81 approved these changes

@eugene123tw eugene123tw eugene123tw approved these changes

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

@sungmanc sungmanc sungmanc approved these changes

@wonjuleee wonjuleee wonjuleee approved these changes

@harimkang harimkang Awaiting requested review from harimkang

@yunchu yunchu Awaiting requested review from yunchu

Assignees
No one assigned
Labels
BUILD DEPENDENCY Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM OTX 2.0 For OTX v2.0 TEST Any changes in tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jaegukhyun @wonjuleee @goodsong81 @eugene123tw @samet-akcay @sungmanc @harimkang