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

Enable hierarchical classification in MPA #1159

Merged
merged 87 commits into from
Jul 20, 2022

Conversation

JihwanEom
Copy link
Contributor

@JihwanEom JihwanEom commented Jul 8, 2022

This PR includes:

Summary

  • Disable torchreid jumping in MPA when the task runs hierarchical classification
  • Add hierarchical model templates for MPA
  • Add hierarchical training capacity in MPA
  • Revised classification inference for stable operation
  • Integrated separated load_annotation method to one method in mpa_cls_dataset.py
  • Set drop_last=True when run hierarchical classification
  • Support hierarchical classification CLI
  • Relocation classification sample datasets in one folder

harimkang and others added 30 commits June 16, 2022 15:35
@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 and removed DEPENDENCY Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM labels Jul 13, 2022
Copy link
Contributor

@harimkang harimkang left a comment

Choose a reason for hiding this comment

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

I left a few comments.
Additionally,

  1. When I looked at the added sample data, I could look at the data sets that already exist, such as 'data/car_tree_bug'. Is there a reason to add an image directly? I think you just need to specify the path in the annotation file, what do you think? (The capacity may increase due to overlapping image data.)
  2. There seems to be a conflict in the submodule of the MPA. Please check.

harimkang
harimkang previously approved these changes Jul 19, 2022
Copy link
Contributor

@harimkang harimkang left a comment

Choose a reason for hiding this comment

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

Some code looks complicated. It would be good to divide it into functions, but it would be good to consider refactoring with SAMClassifier later PR.

jaegukhyun
jaegukhyun previously approved these changes Jul 19, 2022
Copy link
Contributor

@jaegukhyun jaegukhyun left a comment

Choose a reason for hiding this comment

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

LGTM

@JihwanEom JihwanEom dismissed stale reviews from jaegukhyun and harimkang via 2f92279 July 20, 2022 01:22
Copy link
Contributor

@jaegukhyun jaegukhyun left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM.
But please note that we need refactoring in future PRs.
There are too many / long if-else blocks that should be extracted as method or subclassed.

@JihwanEom JihwanEom merged commit 2ba612e into develop Jul 20, 2022
@JihwanEom JihwanEom deleted the jeom/enable-mpa-hierarchical branch July 20, 2022 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ALGO Any changes in OTX Algo Tasks implementation API Any changes in OTX API CLI Any changes in OTE CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants