-
Notifications
You must be signed in to change notification settings - Fork 99
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
Fix track only PredictedInstance #2028
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe changes in the pull request enhance the tracking functionality within the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2028 +/- ##
===========================================
+ Coverage 73.30% 75.47% +2.16%
===========================================
Files 134 133 -1
Lines 24087 24654 +567
===========================================
+ Hits 17658 18607 +949
+ Misses 6429 6047 -382 ☔ View full report in Codecov by Sentry. |
c430585
to
a520942
Compare
Thanks @getzze! This looks like a useful filter, but we do sometimes have use cases where we want to reuse the user-labeled instances, for example, when correcting poses or tracking the same videos you label on. If we can put this behind a feature flag so it's optional (and add some tests), happy to approve! Thanks for the contribution! |
Thanks for reviewing! I will add an option to convert |
b9471f7
to
c9862f2
Compare
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sleap/nn/tracking.py (1)
644-665
: LGTM: Extracted timestep inference logic into a separate method.Good refactoring to move the timestep inference logic from the
track
method into a dedicated method. This improves code organization and reusability.Consider adding docstring examples to illustrate the different cases:
def infer_next_timestep(self, t: Optional[int] = None) -> int: """Infer timestep if not provided. Examples: >>> tracker = Tracker() >>> tracker.infer_next_timestep(5) # Explicit timestep 5 >>> tracker.infer_next_timestep() # No queue, default to 0 0 >>> # With queue, increment from last timestep >>> tracker.track_matching_queue[-1].t = 10 >>> tracker.infer_next_timestep() 11 """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
sleap/gui/learning/runners.py
(1 hunks)sleap/instance.py
(1 hunks)sleap/nn/tracking.py
(10 hunks)
🔇 Additional comments (4)
sleap/gui/learning/runners.py (1)
264-264
: LGTM: Added tracking.only_predicted_instances to boolean conversion list.
The addition of tracking.only_predicted_instances
to the bool_items_as_ints
list ensures proper CLI argument handling for the new tracking option.
sleap/nn/tracking.py (2)
584-584
: LGTM: Added only_predicted_instances attribute.
The new attribute with default value True
aligns with the PR objective to track only predicted instances by default.
1528-1532
: LGTM: Added conditional instance selection based on only_predicted_instances.
The implementation correctly handles both cases:
- When
only_predicted_instances
is True: Uses only predicted instances - When
only_predicted_instances
is False: Converts all instances to predicted instances
This aligns with the PR objectives and the feedback from talmo about supporting both use cases.
sleap/instance.py (1)
1180-1196
: LGTM: Added convert_to_predicted_instance function.
Well-implemented utility function that:
- Handles both Instance and PredictedInstance types
- Preserves existing PredictedInstance objects
- Sets reasonable defaults for score (1.0) and tracking_score (0.0)
- Reuses existing attributes through attr.asdict for clean conversion
I made the proposed changes. One test is failing for no particular reason (it should pass if re-run). |
Description
Force to track only
PredictedInstance
and notInstance
, otherwise we get an error becauseupdate_matched_instance_tracks
only worksPredictedInstance
.This can happen if running
tracking_only
with a dataset that has predicted instances from inference and user-defined instances used for training.Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Bug Fixes