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

BaseKeypointsDataset now inherits from HasPreprocessingParams #1380

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

BloodAxe
Copy link
Contributor

Bugfixes

  • BaseKeypointsDataset now inherits from HasPreprocessingParams to ensure preprocessing params for pose estimation model are saved properly.
  • Changed default confidence threshold from 0.25 to 0.05 to match the default confidence threshold as is training recipe.

Improvement

SupportsReplaceNumClasses and HasPreprocessingParams are no Protocols anymore.
Motivation:

A Protocol class seems to do a lot of black vodoo magic:

class HasPreprocessingParams(Protocol):
   ...

class DetectionDataset(Dataset): # No HasPreprocessingParams
   ...

class COCODetectionDataset(DetectionDataset)

dataset = COCODetectionDataset(...)
isinstance(dataset, HasPreprocessingParams) # Returns True :exploding_head: 

Even not inheriting from HasPreprocessingParams but having the methods with matching signature implement, the isinstance returns True. Which may be really undesired/unexpected/coincidence.

Copy link
Contributor

@Louis-Dupont Louis-Dupont left a comment

Choose a reason for hiding this comment

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

LGTM, just minor notes

tests/unit_tests/preprocessing_unit_test.py Outdated Show resolved Hide resolved
tests/unit_tests/preprocessing_unit_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Louis-Dupont Louis-Dupont left a comment

Choose a reason for hiding this comment

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

LGTM

@BloodAxe BloodAxe merged commit 8c7dc64 into master_320 Aug 21, 2023
@BloodAxe BloodAxe deleted the hotfix/SG-1076-fix-missing-dataset-params branch August 21, 2023 10:41
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.

2 participants