Allow max tracking args for Kalman filter #1986
Conversation
…t` are options in the GUI
…ith both CLI and GUI
…`init_tracker` has `img_hw`
WalkthroughThe changes in this pull request primarily affect the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
sleap/nn/tracking.py (2)
969-970: Clarify the exception message for missing required parameters.The
ValueErrormessage could be more explicit by naming the parameters:raise ValueError("Kalman filter requires `max_tracks` or `target_instance_count` to be set.")This helps users understand which parameters need to be provided.
1403-1403: Remove unused**kwargsparameter from thetrackmethod.The
trackmethod includes**kwargsin its signature, butkwargsis not used within the method. Consider removing it to simplify the method and prevent confusion.
sleap/nn/tracking.py
Outdated
| @@ -574,7 +574,7 @@ class Tracker(BaseTracker): | |||
| max_tracking: bool = False # To enable maximum tracking. | |||
|
|
|||
| cleaner: Optional[Callable] = None # TODO: deprecate | |||
| target_instance_count: int = 0 | |||
| target_instance_count: int = 0 # TODO: deprecate | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to remove dependency on deprecated target_instance_count.
The attribute target_instance_count is marked for deprecation but is still being defined and used. Consider refactoring the code to eliminate its usage and replace it with max_tracks wherever applicable to clean up the codebase and avoid confusion.
sleap/nn/tracking.py
Outdated
| elif (self.target_instance_count or self.max_tracks) and self.post_connect_single_breaks: | ||
| if not self.target_instance_count: | ||
| # If target_instance_count is not set, use max_tracks instead | ||
| # target_instance_count not available in the GUI | ||
| self.target_instance_count = self.max_tracks |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using deprecated target_instance_count in logic.
The code assigns self.target_instance_count from self.max_tracks, despite target_instance_count being marked for deprecation. Refactor the logic to use self.max_tracks directly, removing reliance on the deprecated attribute.
sleap/nn/tracking.py
Outdated
| @@ -850,7 +855,7 @@ | |||
| of_max_levels: int = 3, | |||
| save_shifted_instances: bool = False, | |||
| # Pre-tracking options to cull instances | |||
| target_instance_count: int = 0, | |||
| target_instance_count: int = 0, # TODO: deprecate target_instance_count | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove deprecated parameter target_instance_count from method signature.
The parameter target_instance_count in the make_tracker_by_name method is marked for deprecation but remains in the signature. To streamline the API and prevent future confusion, consider removing it and updating any dependent code to use max_tracks.
sleap/nn/tracking.py
Outdated
| @@ -940,11 +946,17 @@ | |||
| pre_cull_function=pre_cull_function, | |||
| max_tracking=max_tracking, | |||
| max_tracks=max_tracks, | |||
| target_instance_count=target_instance_count, | |||
| target_instance_count=target_instance_count, # TODO: deprecate target_instance_count | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate passing deprecated target_instance_count to the constructor.
When creating tracker_obj, the deprecated target_instance_count is still being passed. Update the constructor and related logic to rely solely on max_tracks, removing the need for the deprecated parameter.
| # Kalman filter requires deprecated target_instance_count | ||
| if (max_tracks or target_instance_count) and kf_init_frame_count: | ||
| if not target_instance_count: | ||
| # If target_instance_count is not set, use max_tracks instead | ||
| # target_instance_count not available in the GUI | ||
| target_instance_count = max_tracks |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update Kalman filter implementation to use max_tracks instead of deprecated target_instance_count.
The Kalman filter initialization depends on target_instance_count, which is marked for deprecation. Modify the Kalman filter code to use max_tracks exclusively, ensuring future maintainability and consistency.
sleap/nn/tracking.py
Outdated
| @@ -1369,6 +1381,8 @@ | |||
| if init_tracker.pre_cull_function is None: | |||
| init_tracker.pre_cull_function = cull_function | |||
|
|
|||
| print(f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters.") | |||
There was a problem hiding this comment.
Fix syntax error due to extra parenthesis in print statement.
There is an extra closing parenthesis at the end of the print statement:
print(f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters.)"Remove the extra parenthesis to correct the syntax error.
Apply this diff:
-print(f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters.)"
+print(f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters.")| connect_single_track_breaks(frames, self.target_instance_count) | ||
| print("Connecting single track breaks.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logging instead of print statements for better practice.
The print statement for status messages can be replaced with the logging module to provide configurable logging levels and outputs.
Apply this diff:
+import logging
...
+logger = logging.getLogger(__name__)
...
-print("Connecting single track breaks.")
+logger.info("Connecting single track breaks.")Committable suggestion was skipped due to low confidence.
| @@ -1420,7 +1435,7 @@ | |||
| # Initialize the Kalman filters | |||
| self.kalman_tracker.init_filters(self.init_set.instances) | |||
|
|
|||
| # print(f"Kalman filters initialized (frame {t})") | |||
| print(f"Kalman filters initialized (frame {t})") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logging instead of print statements for initialization messages.
Replace the print statement with the logging module to improve flexibility and control over log outputs.
Apply this diff:
+import logging
...
+logger = logging.getLogger(__name__)
...
-print(f"Kalman filters initialized (frame {t})")
+logger.info(f"Kalman filters initialized (frame {t})")Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1986 +/- ##
===========================================
+ Coverage 73.30% 75.46% +2.15%
===========================================
Files 134 133 -1
Lines 24087 24642 +555
===========================================
+ Hits 17658 18596 +938
+ Misses 6429 6046 -383 ☔ View full report in Codecov by Sentry. |
eberrigan
changed the title
Description
Replicating the Error
target_instance_countnot present in GUI but is present as argument inmake_tracker_by_namesleap/sleap/nn/tracking.py
Lines 836 to 872 in 1339f0d
Same error when
max_trackingis setTracing the Error
Tracker.make_tracker_by_namedoes taketarget_instance_countas an argument and it is set to0by default.kf_init_frame_countis set to10whiletarget_instance_countremains0, producing the error "Kalman filter requires target instance count."sleap/sleap/nn/tracking.py
Lines 957 to 958 in 1339f0d
target_instance_countandkf_init_frame_countsleap/sleap/nn/tracking.py
Lines 947 to 954 in 1339f0d
Called here (args parsed from CLI):
sleap/sleap/nn/tracking.py
Lines 1564 to 1570 in 1339f0d
presence of
max_trackingdoes change the tracker candidate. Is this a problem?? Will CLI behavior be different than GUI behavior?Related: If we want to cull, we need to add
pre_cull_to_targetoption to the GUI. Right now, nothing will lead to the pre-cull function through the GUIsleap/sleap/nn/tracking.py
Lines 922 to 930 in 1339f0d
Getting the following error for
flowmaxtracksonly (notsimplemaxtracks).This is not being caught in our tests.
More tests need to be added since we didn't catch the args for using the Kalman filter not to be present in the GUI.
Codecov results from first push
Types of changes
Does this address any currently open issues?
#1447
#1583, #1980
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
max_trackingfeature for enhanced control over the number of active tracks.Bug Fixes
target_instance_countusage to ensure compatibility with new tracking attributes.Documentation