-
Notifications
You must be signed in to change notification settings - Fork 4
fix bug saving / loading exported training data #250
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug introduced when ClassifierType was changed from an IntEnum to a string-based Enum. The fix ensures backward compatibility by supporting both the old integer format (1 = Random Forest, 2 = XGBoost) and the new string format when loading training data.
Key Changes:
- Modified
load_training_datato handle both integer and string classifier type values - Updated
export_training_datato save the enum's string value instead of converting to string
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/jabs/project/read_training.py | Added backward-compatible loading logic for classifier_type attribute that handles both old integer and new string formats |
| src/jabs/project/export_training.py | Changed to save classifier_type.value instead of str(classifier_type) for consistent string storage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._video_reader = video_reader | ||
| self._pose_est = pose_est | ||
| self._identity = identity | ||
| self._label_closest = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change related to the rest of the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this shouldn't have been in this PR but it's a non-functional change (self._label_closest is assigned another value a few lines later)
keithshep
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
Recently I changed the ClassifierType from an IntEnum to a an Enum class with string values (inherits from both str and Enum)
This introduced a bug loading the training data in the
jabs-classify traincommand.