Allow max tracking args for Kalman filter #1986
Conversation
WalkthroughThe pull request introduces enhancements to SLEAP's tracking and inference capabilities. Key modifications include adding a Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Tracker
participant Inference
User->>CLI: Specify tracking parameters
CLI->>Tracker: Initialize with max_tracks
Tracker->>Inference: Process tracks
Inference-->>Tracker: Limit track generation
Tracker-->>User: Return tracked results
Poem
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.
sleap/nn/tracking.py
Outdated
| # 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 75.42% 76.13% +0.70%
===========================================
Files 134 134
Lines 24749 24764 +15
===========================================
+ Hits 18668 18854 +186
+ Misses 6081 5910 -171 ☔ View full report in Codecov by Sentry. |
eberrigan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sleap/io/format/coco.py (1)
183-185: LGTM! Consider adding documentation for this edge case.The change correctly preserves spatial information for labeled but invisible keypoints, which is valuable for tracking algorithms. This enhancement aligns well with the PR's objective of improving Kalman filter tracking.
Consider adding a comment explaining this edge case:
if flag == 0: # node not labeled for this instance if (x, y) != (0, 0): + # Some datasets mark points as unlabeled (flag=0) but still provide + # valid coordinates. We preserve this spatial information while + # marking the point as invisible for tracking purposes. points[node] = Point(x, y, False) continue
sleap/io/format/coco.py
Outdated
| if (x, y) != (0, 0): | ||
| # If labeled but invisible, place the node at the coord | ||
| points[node] = Point(x, y, False) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response about the COCO format compliance:
Non-zero coordinates with flag=0 violates COCO keypoint format specification
The COCO keypoint format specifies that when visibility flag=0, the keypoint is not labeled and its coordinates should be (0,0). The current implementation that places points at non-zero coordinates when flag=0 deviates from this specification. This could cause compatibility issues with other COCO-compliant tools.
- In
sleap/io/format/coco.py, the conditionif (x, y) != (0, 0)should be removed, and points with flag=0 should consistently have (0,0) coordinates.
🔗 Analysis chain
Verify compliance with COCO keypoint format specification.
While this change improves tracking capabilities, we should verify that handling flag=0 points with non-zero coordinates aligns with the COCO format specification.
Let me search for more specific information about COCO keypoint handling.
Let me try one more search focusing specifically on keypoint coordinate handling in COCO format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar handling in other COCO implementations
# Look for similar handling in other COCO-related files
rg -A 5 "keypoints.*flag.*0"
# Look for COCO format documentation references
rg -i "coco.*format.*specification"
Length of output: 174
Script:
#!/bin/bash
# Search for COCO keypoint handling patterns
rg -B 2 -A 2 "keypoints.*visibility"
# Search for any comments or documentation about COCO format
rg -B 2 -A 2 "COCO.*format"
# Look for coordinate handling in COCO-related files
rg -B 2 -A 2 "x.*y.*visibility|visibility.*x.*y" sleap/io/format/
Length of output: 592
Script:
#!/bin/bash
# Search for keypoint coordinate handling in COCO format
rg -B 5 -A 5 "keypoints.*=|keypoints.*\[" sleap/io/format/coco.py
# Look for any documentation about visibility flags
rg -B 3 -A 3 "visibility.*flag|flag.*visibility" sleap/io/format/
# Search for coordinate handling patterns
ast-grep --pattern 'if ($x, $y) != (0, 0)'
Length of output: 1070
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sleap/nn/inference.py (1)
1132-1136: Consider improving lambda function readabilityThe lambda function logic is correct but could be more readable. Consider reformatting to improve clarity.
- lambda x: ( - sleap.nn.data.utils.unrag_example(model(x), numpy=False) - if unrag_outputs - else model(x) - ) + lambda x: sleap.nn.data.utils.unrag_example(model(x), numpy=False) + if unrag_outputs + else model(x)sleap_tracking_debug.py (1)
8-13: Add error handling for file operations and data validationThe current implementation lacks error handling which could lead to unclear failures if the file is missing or corrupted.
Consider adding error handling:
-# Load predictions -labels = sleap.load_file(PREDICTIONS_FILE) +try: + labels = sleap.load_file(PREDICTIONS_FILE) + if not labels or not hasattr(labels, 'instances'): + raise ValueError("Invalid or empty predictions file") +except Exception as e: + raise RuntimeError(f"Failed to load predictions from {PREDICTIONS_FILE}: {str(e)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/nn/inference.py(2 hunks)sleap_tracking_debug.py(1 hunks)
🔇 Additional comments (2)
sleap/nn/inference.py (1)
5711-5714: LGTM!
Standard Python idiom for script execution. The implementation is correct and follows best practices.
sleap_tracking_debug.py (1)
15-25: Verify tracker configuration aligns with PR objectives
The configuration includes the new max_tracking and max_tracks parameters along with Kalman filter settings. This appears to address the issue mentioned in PR #1986.
Let's verify the tracker configuration matches the updated API:
Consider making the configuration more flexible:
+# Configuration could be moved to a separate config file or CLI arguments
+tracker_config = {
+ "tracker": "flow",
+ "track_window": 5,
+ "similarity": "instance",
+ "match": "hungarian",
+ "max_tracking": True,
+ "max_tracks": 2,
+ "kf_node_indices": [0, 1],
+ "kf_init_frame_count": 10,
+}
+tracker = sleap.nn.tracking.Tracker.make_tracker_by_name(**tracker_config)✅ Verification successful
Tracker configuration is correctly implemented and documented
The verification confirms that:
- The parameters
max_tracking,max_tracks, andkf_init_frame_countare officially supported and documented in the CLI interface - The documentation shows these parameters are used exactly as implemented in the code:
max_tracking: Controls if tracker should cap max number of tracks (default: False)max_tracks: Sets maximum number of tracks to be tracked (default: None)kf_init_frame_count: Number of frames for Kalman filter initialization (default: 0)
The implementation aligns perfectly with the documented API and PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the tracker parameters match the updated API
ast-grep --pattern 'make_tracker_by_name($$$)' | grep -A 5 'max_tracking\|max_tracks\|kf_init_frame_count'
# Verify if these parameters are documented
rg -A 5 'max_tracking|max_tracks|kf_init_frame_count' --type md
Length of output: 3554
sleap_tracking_debug.py
Outdated
| tracked_lfs = [] | ||
| for lf in labels: | ||
| lf.instances = tracker.track(lf.instances, img=lf.image) | ||
| tracked_lfs.append(lf) | ||
| tracked_labels = sleap.Labels(tracked_lfs) | ||
| tracked_labels.save( | ||
| "/Users/elizabethberrigan/repos/sleap/tests/data/tracks/clip.2node.tracked.slp" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of tracking execution and saving
The tracking and saving implementation could benefit from several improvements:
- Output path has the same portability issues as input path
- No error handling for tracking operations
- No progress indication for potentially long operations
Consider these improvements:
+from tqdm import tqdm
+import os.path as osp
+
tracked_lfs = []
-for lf in labels:
+# Add progress bar for visibility
+for lf in tqdm(labels, desc="Processing frames"):
+ try:
lf.instances = tracker.track(lf.instances, img=lf.image)
tracked_lfs.append(lf)
+ except Exception as e:
+ print(f"Warning: Failed to process frame {lf.frame_idx}: {str(e)}")
+
tracked_labels = sleap.Labels(tracked_lfs)
+
+# Use relative output path
+output_path = osp.join(osp.dirname(__file__), "output", "clip.2node.tracked.slp")
+os.makedirs(osp.dirname(output_path), exist_ok=True)
+
+try:
-tracked_labels.save(
- "/Users/elizabethberrigan/repos/sleap/tests/data/tracks/clip.2node.tracked.slp"
-)
+ tracked_labels.save(output_path)
+except Exception as e:
+ raise RuntimeError(f"Failed to save tracked labels to {output_path}: {str(e)}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tracked_lfs = [] | |
| for lf in labels: | |
| lf.instances = tracker.track(lf.instances, img=lf.image) | |
| tracked_lfs.append(lf) | |
| tracked_labels = sleap.Labels(tracked_lfs) | |
| tracked_labels.save( | |
| "/Users/elizabethberrigan/repos/sleap/tests/data/tracks/clip.2node.tracked.slp" | |
| ) | |
| from tqdm import tqdm | |
| import os.path as osp | |
| tracked_lfs = [] | |
| # Add progress bar for visibility | |
| for lf in tqdm(labels, desc="Processing frames"): | |
| try: | |
| lf.instances = tracker.track(lf.instances, img=lf.image) | |
| tracked_lfs.append(lf) | |
| except Exception as e: | |
| print(f"Warning: Failed to process frame {lf.frame_idx}: {str(e)}") | |
| tracked_labels = sleap.Labels(tracked_lfs) | |
| # Use relative output path | |
| output_path = osp.join(osp.dirname(__file__), "output", "clip.2node.tracked.slp") | |
| os.makedirs(osp.dirname(output_path), exist_ok=True) | |
| try: | |
| tracked_labels.save(output_path) | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to save tracked labels to {output_path}: {str(e)}") |
sleap_tracking_debug.py
Outdated
| PREDICTIONS_FILE = ( | ||
| "/Users/elizabethberrigan/repos/sleap/tests/data/tracks/clip.2node.slp" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded absolute path with a more portable solution
The current hardcoded path contains system-specific information and won't work across different environments.
Consider using one of these approaches:
-PREDICTIONS_FILE = (
- "/Users/elizabethberrigan/repos/sleap/tests/data/tracks/clip.2node.slp"
-)
+# Option 1: Use relative path from script location
+import os
+PREDICTIONS_FILE = os.path.join(os.path.dirname(__file__), "tests/data/tracks/clip.2node.slp")
+
+# Option 2: Use environment variable
+PREDICTIONS_FILE = os.getenv("SLEAP_TEST_DATA_PATH", "tests/data/tracks/clip.2node.slp")Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/nn/test_tracking_integration.py (1)
Line range hint
1-236: Add test coverage for Kalman filter functionality.Given that this PR addresses Kalman filter issues with
target_instance_count, we should add test cases to verify:
- Kalman filter initialization with
kf_init_frame_count- Proper handling of
target_instance_count/max_tracks- Integration with the GUI parameters
Consider adding test cases similar to
test_simple_trackerandtest_simplemax_trackerbut specifically for Kalman filter scenarios. This will help prevent regression issues.Would you like me to help generate these test cases?
🧰 Tools
🪛 Ruff (0.8.2)
149-149: Local variable
kalman_paramsis assigned to but never usedRemove assignment to unused variable
kalman_params(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/nn/test_tracking_integration.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/nn/test_tracking_integration.py
149-149: Local variable kalman_params is assigned to but never used
Remove assignment to unused variable kalman_params
(F841)
| kalman_params = dict( | ||
| kf_node_indices=[0, 1], | ||
| kf_init_frame_count=10, | ||
| ) |
There was a problem hiding this comment.
Remove unused variable or implement Kalman filter tests.
The kalman_params dictionary is defined but never used in the test code. Either:
- Remove the unused variable, or
- Implement test cases to verify the Kalman filter functionality with these parameters.
If you'd like, I can help:
- Remove the unused code:
- kalman_params = dict(
- kf_node_indices=[0, 1],
- kf_init_frame_count=10,
- )- Generate test cases for the Kalman filter functionality that utilize these parameters.
🧰 Tools
🪛 Ruff (0.8.2)
149-149: Local variable kalman_params is assigned to but never used
Remove assignment to unused variable kalman_params
(F841)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
sleap/nn/tracking.py (3)
831-839: Use logging and improve parameter handling.Two suggestions:
- Replace print statement with proper logging
- Consider adding a deprecation warning when target_instance_count is used
Apply this diff:
+import logging +import warnings + +logger = logging.getLogger(__name__) + 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 + else: + warnings.warn( + "target_instance_count is deprecated and will be removed in a future version. " + "Use max_tracks instead.", + DeprecationWarning, + stacklevel=2 + ) connect_single_track_breaks(frames, self.target_instance_count) - print("Connecting single track breaks.") + logger.info("Connecting single track breaks.")
Line range hint
959-995: LGTM! Consider improving error messages.The validation logic for Kalman filter requirements is comprehensive and handles all edge cases correctly. The code properly falls back to max_tracks when target_instance_count is not set.
Consider making the error messages more user-friendly by providing guidance on how to fix the issues:
- raise ValueError( - "Kalman filter requires node indices for instance tracking." - ) + raise ValueError( + "Kalman filter requires node indices for instance tracking. " + "Please specify node indices using --tracking.kf_node_indices." + ) - raise ValueError( - "Kalman filter requires simple tracker for initial tracking." - ) + raise ValueError( + "Kalman filter requires simple tracker for initial tracking. " + "Please use --tracking.tracker simple or simplemaxtracks." + ) - raise ValueError( - "Kalman filter does not support normalized_instance_similarity." - ) + raise ValueError( + "Kalman filter does not support normalized_instance_similarity. " + "Please use a different similarity metric like instance, centroid, or iou." + ) - raise ValueError( - "Kalman filter requires max tracks or target instance count." - ) + raise ValueError( + "Kalman filter requires max tracks or target instance count. " + "Please specify --tracking.max_tracks or --tracking.target_instance_count." + )
1409-1411: Use logging instead of print statement.Replace print statement with proper logging for better control over log levels.
Apply this diff:
+import logging + +logger = logging.getLogger(__name__) + -print( - f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters." -) +logger.info( + f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters." +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/guides/cli.md(1 hunks)sleap/nn/tracking.py(12 hunks)tests/nn/test_tracking_integration.py(1 hunks)
🔇 Additional comments (3)
sleap/nn/tracking.py (1)
1465-1465: Use logging instead of print statement.
Replace print statement with proper logging for better control over log levels.
Apply this diff:
-print(f"Kalman filters initialized (frame {t})")
+logger.info(f"Kalman filters initialized (frame {t})")tests/nn/test_tracking_integration.py (1)
22-181: LGTM! Comprehensive test coverage.
The test function thoroughly covers:
- Error cases for incompatible trackers (flow, flowmaxtracks)
- Error cases for normalized_instance similarity
- Error cases for missing kf_node_indices
- Error cases for missing max_tracks and target_instance_count
- Success cases with various combinations of parameters
The assertions verify that the number of tracks matches the expected count.
docs/guides/cli.md (1)
233-233: LGTM! Clear documentation of Kalman filter requirements.
The documentation accurately describes the requirements for using Kalman filters, matching the implementation in the code:
- Need for kf_node_indices
- Need for max_tracking and max_tracks or target_instance_count
- Requirement for simple or simplemaxtracks tracker
- Incompatibility with normalized_instance similarity
sleap/nn/tracking.py
Outdated
| print(f"Image type before converting: {type(ref_img)}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug print statements and use logging.
Debug print statements should be replaced with proper logging for better control over log levels and output.
Apply this diff:
+import logging
+
+logger = logging.getLogger(__name__)
+
-print(f"Image type before converting: {type(ref_img)}")
+logger.debug(f"Image type before converting: {type(ref_img)}")
-print(f"Image type after converting: {type(ref_img)}")
+logger.debug(f"Image type after converting: {type(ref_img)}")Also applies to: 296-297
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/guides/cli.md (1)
288-292: Consider enhancing the Kalman tracker example.While the example demonstrates the correct usage of Kalman tracker with required parameters, consider these improvements:
- Add a comment explaining why flow tracker is preferred over Kalman
- Include an example with pre_cull_to_target option since it's mentioned in the PR objectives
Apply this diff to enhance the example:
**9. Use Kalman tracker (not recommended since flow is preferred):** +# Note: Flow tracker is preferred as it provides better tracking accuracy and stability + ```none sleap-track -m "models/my_model" --tracking.similarity instance --tracking.tracker simplemaxtracks --tracking.max_tracking 1 --tracking.max_tracks 4 --tracking.kf_init_frame_count 10 --tracking.kf_node_indices 0,1 -o "output_predictions.slp" "input_video.mp4"
+# Alternative: Using Kalman tracker with pre_cull_to_target:
+none +sleap-track -m "models/my_model" --tracking.similarity instance --tracking.tracker simplemaxtracks --tracking.max_tracking 1 --tracking.target_instance_count 4 --tracking.pre_cull_to_target 1 --tracking.kf_init_frame_count 10 --tracking.kf_node_indices 0,1 -o "output_predictions.slp" "input_video.mp4" +</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2342a9d0e10687ca143c332a7eb9ddce91ddccae and f9a047632d52051203219a936bb989f3c323bf4c. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/guides/cli.md` (2 hunks) </details> <details> <summary>🔇 Additional comments (1)</summary> <details> <summary>docs/guides/cli.md (1)</summary> `233-233`: **Documentation accurately describes Kalman filter requirements.** The documentation clearly lists all the required parameters for using Kalman filters, which directly addresses the issue mentioned in PR #1986 regarding the target_instance_count requirement. </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sleap/nn/tracking.py (2)
1405-1407: Use logging instead of print statements.Replace print statement with proper logging for better control over log output.
Apply this diff:
- print( - f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters." - ) + logger = logging.getLogger(__name__) + logger.info( + f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters." + )
1461-1461: Use logging for Kalman filter initialization message.Replace print statement with proper logging.
Apply this diff:
- print(f"Kalman filters initialized (frame {t})") + logger = logging.getLogger(__name__) + logger.info(f"Kalman filters initialized (frame {t})")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/nn/tracking.py(9 hunks)tests/nn/test_tracking_integration.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/nn/test_tracking_integration.py
🔇 Additional comments (2)
sleap/nn/tracking.py (2)
Line range hint 955-991: LGTM! Well-structured validation for Kalman filter requirements.
The validation logic is comprehensive and includes clear error messages for:
- Required node indices
- Incompatible tracker types
- Unsupported similarity functions
- Missing max_tracks or target_instance_count
827-835: 🛠️ Refactor suggestion
Fix indentation and improve logging.
The indentation is inconsistent, and the print statement should be replaced with proper logging.
Apply this diff to fix indentation and use logging:
- 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
- connect_single_track_breaks(frames, self.target_instance_count)
- print("Connecting single track breaks.")
+ 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
+ connect_single_track_breaks(frames, self.target_instance_count)
+ logger = logging.getLogger(__name__)
+ logger.info("Connecting single track breaks.")Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
sleap/info/summary.py (1)
250-280: LGTM! Well-implemented tracking score series calculation.The implementation:
- Follows consistent pattern with other series methods
- Properly handles missing tracking scores and NaN values
- Provides appropriate reduction methods for tracking scores
However, consider adding input validation for the reduction parameter.
Consider adding validation:
def get_tracking_score_series( self, video: Video, reduction: str = "min" ) -> Dict[int, float]: + if reduction not in ["min", "mean"]: + raise ValueError(f"Invalid reduction method: {reduction}. Must be one of: min, mean") reduce_fn = { "min": np.nanmin, "mean": np.nanmean, }[reduction]sleap/gui/dataviews.py (1)
20-21: Consider grouping standard library imports.
The imports from pathlib and typing are standard library modules. It's often best practice to group or order standard library, third-party, and local imports consistently. However, only do so if it aligns with existing conventions in the repository.sleap/gui/widgets/video.py (4)
1195-1211: Pinch gesture logic is straightforward and well-scoped.
Scale factor usage is intuitive and updates the viewer properly. However, ensure that we do not exceed any maximum zoom constraints.if gesture: self.handlePinchGesture(gesture) def handlePinchGesture(self, gesture: QPinchGesture): if gesture.state() == Qt.GestureState.GestureUpdated: factor = gesture.scaleFactor() - self.zoomFactor = max(factor * self.zoomFactor, 1) + # Optionally add an upper bound for self.zoomFactor, e.g., 10 + new_zoom = factor * self.zoomFactor + self.zoomFactor = max(min(new_zoom, 10), 1) self.updateViewer()
1631-1634: Hover events are added but no custom logic is present.
Currently, hoverEnterEvent and hoverLeaveEvent do not contain custom behavior. Verify if we need tooltips or style changes for user feedback.
2018-2023: Hover-based opacity adjustment is elegant.
This logic adjusts overall bounding box opacity based on selection and hover states—straightforward and user-friendly. Consider adding a highlight color or dash style to differentiate from selection.
2317-2317: Ensure box transform updates remain minimal.
Currently, we’re scaling node positions upon mouse release. That is correct, but be sure to test large bounding box changes so that node positions remain valid.Would you like me to provide an automated script to test large bounding box transformations across frames?
sleap/gui/dialogs/frame_range.py (1)
39-42: Add more comprehensive test casesThe main block could include more test cases to verify edge cases.
Consider adding tests for:
- Edge cases (min = max)
- Invalid ranges (min > max)
- Boundary conditions (max_frame_idx = 1)
docs/guides/index.md (1)
33-35: Consider rephrasing for varietyThe documentation addition is good, but consider rephrasing to avoid repetitive "when you want to" pattern.
Suggested revision:
-{ref}`bonsai` when you want to analyze the trained SLEAP model to visualize the poses, centroids and identities for further visual analysis. +{ref}`bonsai` provides tools for analyzing trained SLEAP models, enabling visualization of poses, centroids, and identities for detailed visual analysis.🧰 Tools
🪛 LanguageTool
[style] ~35-~35: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...EAP with Bonsai {ref}bonsaiwhen you want to analyze the trained SLEAP model to visu...(REP_WANT_TO_VB)
sleap/prefs.py (1)
48-55: Enhance error handling and loggingThe current error handling silently ignores file not found errors and might mask other potential issues.
Consider this enhancement:
try: self._prefs = util.get_config_yaml(self._filename) except FileNotFoundError: - pass + logger.debug(f"Preferences file {self._filename} not found, using defaults") + except Exception as e: + logger.warning(f"Error loading preferences: {e}, using defaults") self._prefs = self._prefs or {} for k, v in self._defaults.items(): if k not in self._prefs: + logger.debug(f"Setting default preference for {k}: {v}") self._prefs[k] = vdocs/guides/bonsai.md (2)
11-11: Fix heading level increment.The heading level jumps from h1 to h3. Heading levels should only increment by one level at a time.
-### Exporting a SLEAP trained model +## Exporting a SLEAP trained model🧰 Tools
🪛 Markdownlint (0.37.0)
11-11: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
73-73: Add comma in compound sentence.Add a comma before 'and' as it connects two independent clauses.
-Now you can click the green start button to run the workflow and you can add more modules to analyze and visualize the results in Bonsai. +Now you can click the green start button to run the workflow, and you can add more modules to analyze and visualize the results in Bonsai.🧰 Tools
🪛 LanguageTool
[uncategorized] ~73-~73: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...e green start button to run the workflow and you can add more modules to analyze and...(COMMA_COMPOUND_SENTENCE)
tests/gui/learning/test_dialog.py (1)
436-441: Remove unused variable.The
appvariable is assigned but never used in the test function.- app = MainWindow(no_usage_data=True) ld = LearningDialog( mode="training", labels_filename=Path(min_labels_slp_path), labels=min_labels_slp, )🧰 Tools
🪛 Ruff (0.8.2)
436-436: Local variable
appis assigned to but never usedRemove assignment to unused variable
app(F841)
tests/gui/test_commands.py (1)
855-874: LGTM! Consider enhancing test coverage.The test correctly verifies the basic functionality of
DeleteFrameLimitPredictions. However, consider adding more test cases to cover:
- Edge cases (empty frame range, invalid frame indices)
- Different frame range combinations
- Verification of which instances are actually deleted
sleap/gui/learning/dialog.py (1)
640-653: LGTM! Consider these improvements.The
_validate_id_modelimplementation correctly checks for tracks, but has a few issues:
- Replace type comparison with
isinstance:-if type(inst) == sleap.Instance and inst.track is not None: +if isinstance(inst, sleap.Instance) and inst.track is not None:
- Remove unused variable:
- message = "Cannot run ID model training without tracks." return False🧰 Tools
🪛 Ruff (0.8.2)
643-643: Local variable
messageis assigned to but never usedRemove assignment to unused variable
message(F841)
648-648: Use
isandis notfor type comparisons, orisinstance()for isinstance checks(E721)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
docs/_static/bonsai-connection.jpgis excluded by!**/*.jpgdocs/_static/bonsai-filecapture.jpgis excluded by!**/*.jpgdocs/_static/bonsai-predictcentroids.jpgis excluded by!**/*.jpgdocs/_static/bonsai-predictposeidentities.jpgis excluded by!**/*.jpgdocs/_static/bonsai-predictposes.jpgis excluded by!**/*.jpgdocs/_static/bonsai-workflow.jpgis excluded by!**/*.jpg
📒 Files selected for processing (19)
.github/workflows/website.yml(1 hunks)docs/guides/bonsai.md(1 hunks)docs/guides/index.md(2 hunks)docs/installation.md(4 hunks)sleap/config/frame_range_form.yaml(1 hunks)sleap/gui/app.py(6 hunks)sleap/gui/commands.py(3 hunks)sleap/gui/dataviews.py(2 hunks)sleap/gui/dialogs/frame_range.py(1 hunks)sleap/gui/learning/dialog.py(2 hunks)sleap/gui/widgets/video.py(9 hunks)sleap/info/summary.py(7 hunks)sleap/instance.py(10 hunks)sleap/nn/inference.py(5 hunks)sleap/prefs.py(2 hunks)tests/fixtures/datasets.py(1 hunks)tests/gui/learning/test_dialog.py(2 hunks)tests/gui/test_commands.py(2 hunks)tests/info/test_summary.py(1 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- sleap/gui/app.py
- sleap/gui/commands.py
- sleap/nn/inference.py
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/learning/dialog.py
643-643: Local variable message is assigned to but never used
Remove assignment to unused variable message
(F841)
648-648: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
sleap/gui/dialogs/frame_range.py
27-27: Local variable min_frame_idx_field is assigned to but never used
Remove assignment to unused variable min_frame_idx_field
(F841)
34-34: Local variable max_frame_idx_field is assigned to but never used
Remove assignment to unused variable max_frame_idx_field
(F841)
tests/gui/learning/test_dialog.py
436-436: Local variable app is assigned to but never used
Remove assignment to unused variable app
(F841)
sleap/gui/dataviews.py
395-395: Undefined name Video
(F821)
sleap/nn/inference.py
4459-4459: Loop control variable image not used within loop body
Rename unused image to _image
(B007)
🪛 yamllint (1.35.1)
sleap/config/frame_range_form.yaml
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
🪛 LanguageTool
docs/guides/bonsai.md
[uncategorized] ~73-~73: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...e green start button to run the workflow and you can add more modules to analyze and...
(COMMA_COMPOUND_SENTENCE)
docs/guides/index.md
[style] ~35-~35: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...EAP with Bonsai {ref}bonsai when you want to analyze the trained SLEAP model to visu...
(REP_WANT_TO_VB)
🪛 Markdownlint (0.37.0)
docs/guides/bonsai.md
11-11: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🔇 Additional comments (18)
sleap/instance.py (1)
Line range hint 1052-1076: LGTM! Consistent implementation of tracking score parameter.
The tracking score parameter has been properly added across all factory methods with consistent default values and parameter passing. This change aligns with the PR objectives for fixing the Kalman filter tracking functionality.
Also applies to: 1087-1087, 1102-1102, 1123-1123, 1134-1134, 1149-1149, 1160-1160, 1171-1171, 1186-1186, 1197-1197
tests/info/test_summary.py (1)
44-55: LGTM! Comprehensive test coverage for tracking scores.
The test thoroughly validates the new get_tracking_score_series functionality:
- Tests both reduction methods ("min" and "mean")
- Verifies expected array length
- Validates specific score values at different frames
sleap/info/summary.py (2)
24-24: LGTM! Documentation formatting improvements.
Consistent use of backticks for class references improves readability.
Also applies to: 44-44, 70-70, 96-96, 124-124, 229-229
250-280: Verify integration with Kalman filter tracking.
Let's verify that this implementation properly supports the Kalman filter tracking functionality mentioned in the PR objectives.
✅ Verification successful
Implementation properly supports tracking functionality
The verification shows that:
- The tracking score implementation is properly integrated into the tracking system as evidenced by the tracking score assignment in
sleap/nn/tracking.py - The functionality is well-tested as shown in
tests/info/test_summary.pywhich verifies the tracking score series calculation - The
get_tracking_score_seriesmethod correctly aggregates tracking scores across frames, supporting the tracking system's needs - The implementation allows for both minimum and mean score calculations, providing flexibility in tracking score analysis
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Kalman filter integration with tracking scores
# Check for Kalman filter usage of tracking scores
rg -l "tracking_score.*Kalman" --type py
# Check for related configuration or parameter handling
rg -l "target_instance_count.*Kalman" --type py
Length of output: 92
Script:
#!/bin/bash
# Let's search for Kalman filter implementation and tracking score usage separately
# Find files containing Kalman filter implementation
rg -l "class.*Kalman" --type py
# Find files containing tracking score usage
rg -l "tracking_score" --type py
# Search for tracking-related configuration
rg -l "tracking.*config" --type py
Length of output: 443
Script:
#!/bin/bash
# Let's examine the Kalman filter implementation and its relationship with tracking scores
# Check Kalman filter implementation for tracking score usage
rg -A 5 -B 5 "tracking_score" sleap/nn/tracker/kalman.py
# Check tracking.py for the connection between Kalman and tracking scores
rg -A 5 -B 5 "tracking_score" sleap/nn/tracking.py
# Look at the instance class to understand tracking score implementation
ast-grep --pattern 'class LabeledInstance {
$$$
tracking_score
$$$
}'
Length of output: 504
Script:
#!/bin/bash
# Let's examine the tracking implementation more closely
# Look for tracking score assignment in tracking.py
ast-grep --pattern 'def track($$$) {
$$$
tracking_score
$$$
}'
# Check test coverage for tracking scores
rg -A 5 -B 5 "test.*tracking_score" tests/info/test_summary.py
# Look for the relationship between tracking and instances
rg -A 5 "def track" sleap/nn/tracking.py
Length of output: 1008
sleap/gui/dataviews.py (3)
23-24: Imports look fine.
No issues found with importing numpy or Qt modules.
386-394: Property list aligns well with the new “name” and “filepath” fields.
The updated tuple for properties is clear and coherent.
27-28: Ensure circular dependencies are avoided.
These imports from the sleap.gui package appear top-level. Verify that no cross-module references cause circular import issues, especially since these appear in a core GUI model.
✅ Verification successful
Based on the analysis of the import patterns and content of the files, I can now provide the final response:
No circular dependency issues detected in these imports.
The imports are safe as the dependency graph is unidirectional:
state.pyis a foundational module with no GUI-related importsdataviews.pyimports fromstate.pyandcommands.py- No module that imports
dataviews.pyis imported bydataviews.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking for circular imports that involve sleap/gui/dataviews.py and sleap/gui/commands.py or sleap/gui/state.py
# We'll search for import chains that might produce cycles.
ast-grep --pattern $'import $_ from sleap.gui.commands'
ast-grep --pattern $'import $_ from sleap.gui.state'
Length of output: 110
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find import patterns
echo "=== Checking imports in sleap/gui/dataviews.py ==="
rg "^from.*import|^import" sleap/gui/dataviews.py
echo -e "\n=== Checking where dataviews.py is imported ==="
rg "from.*dataviews.*import|import.*dataviews" --type py
echo -e "\n=== Checking imports in sleap/gui/state.py ==="
rg "^from.*import|^import" sleap/gui/state.py
echo -e "\n=== Checking where state.py is imported ==="
rg "from.*state.*import|import.*state" --type py
Length of output: 1698
Script:
#!/bin/bash
# Let's check the content of commands.py to see if it imports dataviews
echo "=== Checking imports in sleap/gui/commands.py ==="
rg "^from.*import|^import" sleap/gui/commands.py
# Let's also check the content of state.py to see what it contains
echo -e "\n=== Content of sleap/gui/state.py ==="
cat sleap/gui/state.py
Length of output: 8163
sleap/gui/widgets/video.py (4)
65-65: Good addition for pinch gesture support.
This import will facilitate zoom features, which enhances user experience.
1834-1834: No immediate issues with _is_hovering initialization.
This boolean is tracked to detect hover state changes.
1856-1856: Accepting hover events can be beneficial.
The call to setAcceptHoverEvents(True) ensures that hover events are dispatched.
2117-2126: Hover event overrides in QtInstance.
The code is consistent with the approach in Qt. The user will get real-time highlight feedback. Looks good.
.github/workflows/website.yml (1)
11-11: Branch trigger updated to 'liezl/add-channels-for-pip-conda-env'.
This is a simple and valid workflow config change. Ensure the relevant branch name is correct and that no other references remain to the old branch.
sleap/prefs.py (1)
31-32: LGTM: New default preferences
The new default preferences for node marker and label sizes are well-structured and provide good granularity for UI customization.
docs/guides/bonsai.md (1)
1-75: Well-structured and comprehensive documentation!
The documentation provides clear instructions for using Bonsai with SLEAP models, including installation steps, module usage, and workflow configuration. The inclusion of visual aids enhances understanding.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~73-~73: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...e green start button to run the workflow and you can add more modules to analyze and...
(COMMA_COMPOUND_SENTENCE)
🪛 Markdownlint (0.37.0)
11-11: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
tests/fixtures/datasets.py (1)
100-112: Well-implemented test fixture!
The fixture is properly documented with generation instructions and follows the established pattern of other fixtures in the file. The implementation is clean and consistent.
tests/gui/learning/test_dialog.py (1)
435-451: Well-structured test implementation!
The test thoroughly validates the ID model under different conditions:
- Initial state without tracks
- With track but unassigned
- With track assigned to instances
The test progression and assertions are clear and effective.
🧰 Tools
🪛 Ruff (0.8.2)
436-436: Local variable app is assigned to but never used
Remove assignment to unused variable app
(F841)
docs/installation.md (1)
30-30: LGTM! Good documentation enhancement.
Adding a note about alternative terminal apps like Cmder and Windows Terminal improves the user experience for Windows users.
sleap/gui/learning/dialog.py (1)
672-680: LGTM! Good integration with pipeline validation.
The changes correctly integrate ID model validation into the pipeline validation process.
sleap/gui/dataviews.py
Outdated
| def item_to_data(self, obj, item: "Video"): | ||
| data = {} | ||
| for property in self.properties: | ||
| if property == "name": | ||
| data[property] = Path(item.filename).name | ||
| elif property == "filepath": | ||
| data[property] = str(Path(item.filename).parent) | ||
| else: | ||
| data[property] = getattr(item, property) | ||
| return data |
There was a problem hiding this comment.
Address missing type annotation reference "Video".
You have “item: ‘Video’” in the signature, but “Video” is neither imported at the top of this file nor aliased locally. This causes a name error in some static checks.
Apply the following diff to fix the name error:
+from sleap.io.video import Video
def item_to_data(self, obj, item: "Video"):
data = {}
...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.properties: | |
| if property == "name": | |
| data[property] = Path(item.filename).name | |
| elif property == "filepath": | |
| data[property] = str(Path(item.filename).parent) | |
| else: | |
| data[property] = getattr(item, property) | |
| return data | |
| from sleap.io.video import Video | |
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.properties: | |
| if property == "name": | |
| data[property] = Path(item.filename).name | |
| elif property == "filepath": | |
| data[property] = str(Path(item.filename).parent) | |
| else: | |
| data[property] = getattr(item, property) | |
| return data |
🧰 Tools
🪛 Ruff (0.8.2)
395-395: Undefined name Video
(F821)
sleap/gui/widgets/video.py
Outdated
| self.grabGesture(Qt.GestureType.PinchGesture) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis, I can now provide the final response:
Gesture handling needs test coverage
The pinch gesture implementation in video.py lacks test coverage. While the codebase has tests for the video player widget including mouse wheel events, there are no tests specifically covering gesture interactions. Key areas needing test coverage:
handleGestureEventmethodhandlePinchGesturemethod- Zoom factor changes through pinch gestures
- Gesture state handling
🔗 Analysis chain
Ensure gesture grabbing is properly tested.
Grabbing the pinch gesture can interfere with other gestures on certain platforms or older Qt versions. Validate in multiple environments if possible.
Let me search for gesture-related code and tests to better understand the implementation and testing coverage.
Let me check the test files to see if they cover gesture handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for usage references to "grabGesture" within the codebase to ensure there's no conflict
rg "grabGesture"
Length of output: 97
Script:
#!/bin/bash
# Search for gesture-related code and tests
echo "=== Searching for gesture-related code ==="
rg -i "gesture" -A 3
echo -e "\n=== Searching for test files related to video widget ==="
fd "test.*video.*\.py$"
Length of output: 1750
Script:
#!/bin/bash
# Check test coverage for gesture handling
echo "=== Checking video player tests ==="
cat tests/gui/test_video_player.py
echo -e "\n=== Looking for any Qt gesture tests in the entire codebase ==="
rg -i "test.*gesture|gesture.*test" -A 3
Length of output: 5712
sleap/config/frame_range_form.yaml
Outdated
| main: | ||
|
|
||
| - name: min_frame_idx | ||
| label: Minimum frame index | ||
| type: int | ||
| range: 1,1000000 | ||
| default: 1 | ||
|
|
||
| - name: max_frame_idx | ||
| label: Maximum frame index | ||
| type: int | ||
| range: 1,1000000 | ||
| default: 1000 |
There was a problem hiding this comment.
Add a blank line at file end to satisfy lint.
YAML rules often require a trailing newline.
range: 1,1000000
default: 1000
+ 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| main: | |
| - name: min_frame_idx | |
| label: Minimum frame index | |
| type: int | |
| range: 1,1000000 | |
| default: 1 | |
| - name: max_frame_idx | |
| label: Maximum frame index | |
| type: int | |
| range: 1,1000000 | |
| default: 1000 | |
| main: | |
| - name: min_frame_idx | |
| label: Minimum frame index | |
| type: int | |
| range: 1,1000000 | |
| default: 1 | |
| - name: max_frame_idx | |
| label: Maximum frame index | |
| type: int | |
| range: 1,1000000 | |
| default: 1000 | |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
sleap/gui/dialogs/frame_range.py
Outdated
| def _update_max_frame_range(self, value): | ||
| min_frame_idx_field = self.form_widget.fields["min_frame_idx"] | ||
| max_frame_idx_field = self.form_widget.fields["max_frame_idx"] | ||
|
|
||
| max_frame_idx_field.setRange(value, max_frame_idx_field.maximum()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable and improve range validation
The min_frame_idx_field variable is declared but never used. Additionally, the range validation could be more robust.
Apply this diff:
def _update_max_frame_range(self, value):
- min_frame_idx_field = self.form_widget.fields["min_frame_idx"]
max_frame_idx_field = self.form_widget.fields["max_frame_idx"]
- max_frame_idx_field.setRange(value, max_frame_idx_field.maximum())
+ current_max = max_frame_idx_field.value()
+ max_frame_idx_field.setRange(value, max_frame_idx_field.maximum())
+ if current_max < value:
+ max_frame_idx_field.setValue(value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _update_max_frame_range(self, value): | |
| min_frame_idx_field = self.form_widget.fields["min_frame_idx"] | |
| max_frame_idx_field = self.form_widget.fields["max_frame_idx"] | |
| max_frame_idx_field.setRange(value, max_frame_idx_field.maximum()) | |
| def _update_max_frame_range(self, value): | |
| max_frame_idx_field = self.form_widget.fields["max_frame_idx"] | |
| current_max = max_frame_idx_field.value() | |
| max_frame_idx_field.setRange(value, max_frame_idx_field.maximum()) | |
| if current_max < value: | |
| max_frame_idx_field.setValue(value) |
🧰 Tools
🪛 Ruff (0.8.2)
27-27: Local variable min_frame_idx_field is assigned to but never used
Remove assignment to unused variable min_frame_idx_field
(F841)
sleap/gui/dialogs/frame_range.py
Outdated
| def _update_min_frame_range(self, value): | ||
| min_frame_idx_field = self.form_widget.fields["min_frame_idx"] | ||
| max_frame_idx_field = self.form_widget.fields["max_frame_idx"] | ||
|
|
||
| min_frame_idx_field.setRange(min_frame_idx_field.minimum(), value) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable and improve range validation
Similar to above, max_frame_idx_field is unused and range validation could be improved.
Apply this diff:
def _update_min_frame_range(self, value):
min_frame_idx_field = self.form_widget.fields["min_frame_idx"]
- max_frame_idx_field = self.form_widget.fields["max_frame_idx"]
- min_frame_idx_field.setRange(min_frame_idx_field.minimum(), value)
+ current_min = min_frame_idx_field.value()
+ min_frame_idx_field.setRange(min_frame_idx_field.minimum(), value)
+ if current_min > value:
+ min_frame_idx_field.setValue(value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _update_min_frame_range(self, value): | |
| min_frame_idx_field = self.form_widget.fields["min_frame_idx"] | |
| max_frame_idx_field = self.form_widget.fields["max_frame_idx"] | |
| min_frame_idx_field.setRange(min_frame_idx_field.minimum(), value) | |
| def _update_min_frame_range(self, value): | |
| min_frame_idx_field = self.form_widget.fields["min_frame_idx"] | |
| current_min = min_frame_idx_field.value() | |
| min_frame_idx_field.setRange(min_frame_idx_field.minimum(), value) | |
| if current_min > value: | |
| min_frame_idx_field.setValue(value) |
🧰 Tools
🪛 Ruff (0.8.2)
34-34: Local variable max_frame_idx_field is assigned to but never used
Remove assignment to unused variable max_frame_idx_field
(F841)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/ISSUE_TEMPLATE/bug_report.md (1)
32-36: Adjust list indentation for better markdown compatibilityThe installation method checklist is well-structured with proper links, but the indentation should be adjusted to follow markdown best practices.
Apply this change to use 2-space indentation for the nested list:
- - [ ] [Conda from package](https://sleap.ai/installation.html#conda-package) - - [ ] [Conda from source](https://sleap.ai/installation.html#conda-from-source) - - [ ] [pip package](https://sleap.ai/installation.html#pip-package) - - [ ] [Apple Silicon Macs](https://sleap.ai/installation.html#apple-silicon-macs) + - [ ] [Conda from package](https://sleap.ai/installation.html#conda-package) + - [ ] [Conda from source](https://sleap.ai/installation.html#conda-from-source) + - [ ] [pip package](https://sleap.ai/installation.html#pip-package) + - [ ] [Apple Silicon Macs](https://sleap.ai/installation.html#apple-silicon-macs)🧰 Tools
🪛 Markdownlint (0.37.0)
33-33: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
34-34: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
35-35: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
36-36: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
docs/conf.py (1)
31-31: Consider retaining dynamic version readingWhile the version numbers are correctly synchronized with version.py, consider keeping the commented-out dynamic version reading code. This would:
- Ensure versions stay in sync automatically
- Reduce the risk of version mismatches
- Simplify future version updates
Uncomment and update the version reading code:
-version = "1.4.1" +# Get the sleap version +with open("../sleap/version.py") as f: + version_file = f.read() + version = re.search("\d.+(?=['\"])", version_file).group(0)Also applies to: 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/workflows/build_conda_ci.yml(1 hunks).github/workflows/build_pypi_ci.yml(1 hunks).github/workflows/website.yml(1 hunks)docs/conf.py(2 hunks)docs/installation.md(7 hunks)sleap/version.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/build_pypi_ci.yml
- .github/workflows/build_conda_ci.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/website.yml
- docs/installation.md
🧰 Additional context used
🪛 Markdownlint (0.37.0)
.github/ISSUE_TEMPLATE/bug_report.md
33-33: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
34-34: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
35-35: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
36-36: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (3)
.github/ISSUE_TEMPLATE/bug_report.md (1)
28-36: Well-structured template improvements for tracking bug reports
The enhanced system setup section will be particularly helpful for capturing environment details needed to debug tracking-related issues, such as the Kalman filter problems addressed in this PR. The clear structure and specific version fields will make it easier to reproduce and resolve similar issues in the future.
🧰 Tools
🪛 Markdownlint (0.37.0)
33-33: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
34-34: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
35-35: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
36-36: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
sleap/version.py (1)
14-14: Consider deferring the version bump to 1.4.1
The version bump from "1.4.1a2" to "1.4.1" indicates a transition from alpha to stable release. However, this PR is actively fixing a critical Kalman filter tracking functionality. Consider:
- Keeping the alpha version until the tracking fixes are merged and tested
- Creating a separate PR for the version bump after confirming stability
Let's check if there are any other pending PRs or issues that might affect stability:
✅ Verification successful
Let me gather more information about version-related changes and recent bug fixes.
Let me check the test file and any recent changes to version-related files.
Based on the gathered information, I can now generate the final response:
Version bump to 1.4.1 appears appropriate
The version change from "1.4.1a2" to "1.4.1" is justified because:
- No open PRs or issues with bug/critical labels exist
- The version is widely integrated across the codebase for tracking, training, and diagnostics
- The version format in setup.py confirms this follows the project's versioning pattern
- The test suite includes version validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for open PRs and issues with labels indicating bugs or critical issues
gh pr list --state open --label "bug,critical" --json title,url
gh issue list --state open --label "bug,critical" --json title,url
Length of output: 136
Script:
#!/bin/bash
# Check recent commits and PR description for context about version changes and bug fixes
gh pr view --json title,body,commits
# Check for any changelog or release notes
fd "CHANGELOG|HISTORY|RELEASES" --type f
# Look for any version-related files or tests
fd "version|release" --type f
Length of output: 204
Script:
#!/bin/bash
# Check version test file content
cat tests/test_version.py
# Check git history for version.py to understand recent changes
git log -n 5 --oneline -- sleap/version.py
# Look for any version imports or usage in the codebase
rg "__version__" --type py -C 2
Length of output: 5345
docs/conf.py (1)
181-181: LGTM: Minor style update
The change from single to double quotes in html_css_files is consistent with Python style guides.
.github/ISSUE_TEMPLATE/bug_report.md
Outdated
| - Version(s): | ||
| <!-- e.g. [SLEAP v1.4.1, python 3.8] ---> |
There was a problem hiding this comment.
Fix HTML comment syntax in version example
The HTML comment in the version example uses incorrect syntax for closing the comment.
Apply this change:
-<!-- e.g. [SLEAP v1.4.1, python 3.8] --->
+<!-- e.g. [SLEAP v1.4.1, python 3.8] -->📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Version(s): | |
| <!-- e.g. [SLEAP v1.4.1, python 3.8] ---> | |
| - Version(s): | |
| <!-- e.g. [SLEAP v1.4.1, python 3.8] --> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.rst (1)
72-72: Reconsider adding dev channel to main installation instructions.While this change helps users access the Kalman filter fix, adding a development channel to the main installation instructions may expose users to unstable features. Consider:
- Adding a note explaining why the dev channel is temporarily needed
- Documenting when this fix will be available in the stable channel
- Moving this to a separate "Development Version" section
Here's a suggested approach:
- conda create -y -n sleap -c conda-forge -c nvidia -c sleap/label/dev -c sleap -c anaconda sleap + conda create -y -n sleap -c conda-forge -c nvidia -c sleap -c anaconda sleap + Development Version + ^^^^^^^^^^^^^^^^^ + To access the latest fixes (including Kalman filter improvements), use: + + .. code-block:: bash + + conda create -y -n sleap -c conda-forge -c nvidia -c sleap/label/dev -c sleap -c anaconda sleap + + Note: The development version may include unstable features. These fixes will be available + in the next stable release (v1.4.2).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.rst(1 hunks)docs/installation.md(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/installation.md
eberrigan
force-pushed
the
elizabeth/Set-Kalman-filter-args-equal-to-max-tracking-args
branch
from
e68928f to
32aa5ee
Compare
…t` are options in the GUI
…ith both CLI and GUI
…`init_tracker` has `img_hw`
…hing besides normalized
eberrigan
force-pushed
the
elizabeth/Set-Kalman-filter-args-equal-to-max-tracking-args
branch
from
32aa5ee to
fa5a82b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
sleap/nn/tracking.py (4)
827-835: Use logging instead of print statement.Replace the print statement with proper logging for better control over log levels and output.
Apply this change:
- print("Connecting single track breaks.") + logger = logging.getLogger(__name__) + logger.info("Connecting single track breaks.")
Line range hint
931-951: Document GUI limitations in docstring.The comment indicates GUI accessibility limitations. This should be properly documented in the method's docstring.
Add this to the method's docstring:
""" Notes: The pre-culling functionality using target_instance_count is currently not accessible through the GUI. Use max_tracks parameter instead when working with the GUI. """
Line range hint
955-991: Improve error messages for Kalman filter requirements.The error messages could be more descriptive to help users understand the requirements and alternatives.
Apply these changes:
- raise ValueError( - "Kalman filter requires node indices for instance tracking." - ) + raise ValueError( + "Kalman filter requires node indices for instance tracking. " + "Please specify node_indices parameter." + ) - raise ValueError( - "Kalman filter requires simple tracker for initial tracking." - ) + raise ValueError( + "Kalman filter requires simple tracker for initial tracking. " + "Please use 'simple' or 'simplemaxtracks' tracker instead of flow-based trackers." + ) - raise ValueError( - "Kalman filter does not support normalized_instance_similarity." - ) + raise ValueError( + "Kalman filter does not support normalized_instance_similarity. " + "Please use a different similarity function." + ) - raise ValueError( - "Kalman filter requires max tracks or target instance count." - ) + raise ValueError( + "Kalman filter requires either max_tracks or target_instance_count parameter. " + "Please specify one of these parameters." + )
1405-1407: Use logging instead of print statement for Kalman filter initialization.Replace the print statement with proper logging for better control over log levels and output.
Apply this change:
- print( - f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters." - ) + logger = logging.getLogger(__name__) + logger.info( + f"Using {init_tracker.get_name()} to track {init_frame_count} frames for Kalman filters." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/guides/cli.md(2 hunks)sleap/nn/inference.py(2 hunks)sleap/nn/tracker/kalman.py(1 hunks)sleap/nn/tracking.py(9 hunks)tests/nn/test_tracking_integration.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- sleap/nn/tracker/kalman.py
- tests/nn/test_tracking_integration.py
- docs/guides/cli.md
🔇 Additional comments (2)
sleap/nn/tracking.py (1)
Line range hint 5722-5725: LGTM: Standard main module check implementation.
The main module check is correctly implemented using the standard Python idiom.
sleap/nn/inference.py (1)
1132-1136: LGTM: Correct implementation of conditional unragging.
The conditional unragging of outputs is correctly implemented using a lambda function.
| @@ -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
Implement proper deprecation warning for target_instance_count.
Since target_instance_count is marked for deprecation but still being used, implement a proper deprecation warning to notify users.
Add this warning in the __init__ method:
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
if self.target_instance_count:
import warnings
warnings.warn(
"target_instance_count is deprecated and will be removed in a future version. "
"Use max_tracks instead.",
DeprecationWarning,
stacklevel=2
)
|
Closes #1980 |
eberrigan
deleted the
elizabeth/Set-Kalman-filter-args-equal-to-max-tracking-args
branch
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
and is required for the Kalman Filter classmethod
sleap/sleap/nn/tracking.py
Lines 1331 to 1338 in 47a0efa
"Kalman filter requires target instance count.".Same error when
max_trackingis setmax_tracksis configurable from the GUI.Tracing the Error
Tracker.make_tracker_by_namedoes taketarget_instance_countas an argument and it is set to0by default.kf_init_frame_countis set to10in the GUI when people want to use the Kalman filter, whiletarget_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. If people want to keep the exact same behavior as they have now, they will have to use the CLI to set thetarget_instance_countwithout settingmax_tracks.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
flowtrackers and trackers using thenormalized_instanceresult in the error where the image is an integer type and not a numpy array. This happens because ofsleap/sleap/nn/tracking.py
Line 1412 in 47a0efa
init_trackerisflow,flowmaxtracksuntracked_instancesand frame index but not image. For these trackers to work we would have to pass the frame index to the Kalman tracker and the image to the initial trackersleap/sleap/nn/tracking.py
Lines 1465 to 1467 in 15deb3f
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_trackingoption to limit the number of active tracks in the tracking system.Bug Fixes
target_instance_count.Tests
Documentation