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

Feat: replace detect_ids with detect #653

Merged
merged 6 commits into from
Jun 8, 2022

Conversation

ongtw
Copy link
Contributor

@ongtw ongtw commented Jun 6, 2022

Changes:

  • Replace detect_ids with detect so as to be more user-friendly
    • Instead of detect_ids: ["dog"], it is now detect: ["dog"]
    • Replace detect_ids in object detection models (efficientdet, yolo, yolox) configs
    • Update tests to use detect in place of detect_ids
    • Update docs and use cases yaml files
  • Add deprecation warning notice for detect_ids.

Technotes:

  • The changes stop at peekingduck/pipeline/nodes/model level, within the __init__ of EfficientDet, Yolo and YoloX, relevant code shown below:
        self.detect_ids = config["detect"]  # change "detect_ids" to "detect"
  • Internally, each object detection model still uses self.detect_ids.
  • detect is used at the user-interface level.

@ongtw ongtw changed the title Feat model detect Feat: replace detect_ids with detect Jun 6, 2022
@ongtw ongtw self-assigned this Jun 6, 2022
@ongtw ongtw added the enhancement New feature or request label Jun 6, 2022
Copy link
Contributor

@liyier90 liyier90 left a comment

Choose a reason for hiding this comment

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

Some comments for your consideration

Edit: I realized detect_ids is referenced in use cases (such as face_mask_detection) and in the yolo_face model, I think those should be changed to detect as well

ongtw added 2 commits June 7, 2022 22:44
…e of 'detect_ids'

- update declarative_loader to use 'deprecate' for 'detect_ids' deprecation notice
- update declarative_loader to perform class names to id conversion if model is not yolo_face
@ongtw
Copy link
Contributor Author

ongtw commented Jun 7, 2022

Also updated yolo_face to use detect instead of detect_ids.
Didn't modify yolo_face initially as it only takes in [0, 1] and does not have any class names (no no_mask or masked labels).
After thinking about it, decided to update as you pointed out to keep everything consistent.

@liyier90 liyier90 merged commit 310ca55 into aisingapore:main Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants