Add normalized_instance_similarity method
#1939
Conversation
WalkthroughThe pull request introduces enhancements to the tracking feature by adding new similarity metrics, "normalized_instance" and "object_keypoint," across various components. These changes are reflected in the command-line interface documentation, configuration files, and implementation of new functions. The updates improve the methods available for measuring similarity, thereby refining the tracking and analysis processes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant Tracker
participant Test
User->>CLI: Request tracking with normalized_instance
CLI->>Config: Fetch similarity options
Config->>Tracker: Set normalized_instance for tracking
Tracker->>Tracker: Compute similarity using normalized_instance
Tracker->>Test: Validate tracking with new similarity metric
Possibly related PRs
Suggested labels
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 (
|
gitttt-1234
changed the title
object_keypoint method for similarity score computation
gitttt-1234
changed the title
object_keypoint method for similarity score computationnormalized_instance_similarity method
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1939 +/- ##
===========================================
+ Coverage 73.30% 75.37% +2.06%
===========================================
Files 134 133 -1
Lines 24087 24480 +393
===========================================
+ Hits 17658 18452 +794
+ Misses 6429 6028 -401 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- docs/guides/cli.md (1 hunks)
- docs/guides/proofreading.md (1 hunks)
- sleap/config/pipeline_form.yaml (2 hunks)
- sleap/nn/tracker/components.py (2 hunks)
- sleap/nn/tracking.py (2 hunks)
- tests/nn/test_tracker_components.py (1 hunks)
- tests/nn/test_tracking_integration.py (1 hunks)
Additional context used
Ruff
sleap/nn/tracking.py
8-8:
functoolsimported but unusedRemove unused import:
functools(F401)
Additional comments not posted (9)
tests/nn/test_tracking_integration.py (1)
141-141: LGTM!The code change to include the new
normalized_instancesimilarity method is approved. This change is consistent with the PR summary and expands the set of similarity metrics that can be tested within themainfunction.docs/guides/proofreading.md (1)
53-53: LGTM!The documentation change to include
"**normalized_instance**"as a new method for measuring similarity in instances is approved. This change is consistent with the AI-generated summary and expands the existing methods of similarity measurement by providing an additional layer of normalization that could improve the accuracy of instance matching.tests/nn/test_tracker_components.py (1)
40-40: LGTM!The code change to include
"normalized_instance"in the@pytest.mark.parametrizedecorator for thesimilarityparameter is approved. This change expands the set of similarity metrics that can be tested within thetest_tracker_by_namefunction, allowing for more comprehensive validation of the tracker behavior under varied conditions. The overall logic and control flow of the test function remain unchanged.docs/guides/cli.md (1)
210-210: LGTM!The documentation update is consistent with the new
normalized_instance_similaritymethod introduced in the code.sleap/config/pipeline_form.yaml (2)
442-442: LGTM!The configuration update is consistent with the new
normalized_instance_similaritymethod introduced in the code.
541-541: This is a duplicate of the previous configuration update. No need to comment again.sleap/nn/tracker/components.py (2)
33-47: LGTM!The new
normalized_instance_similarityfunction looks good:
- It correctly normalizes the keypoints using the maximum x and y coordinates from both instances.
- The similarity computation using squared differences and averaging over visible reference points is a reasonable approach.
- The function is well-documented with a clear docstring.
55-55: LGTM!The change in the
instance_similarityfunction improves the readability by removing unnecessary line breaks. The core logic remains unchanged.sleap/nn/tracking.py (1)
500-501: LGTM!The changes to the
get_candidatesfunction look good:
- The new
normalized_instanceandobject_keypointparameters have been added to the function signature.object_keypointis now assigned the value offactory_object_keypoint_similarity()instead ofinstance_similarity, indicating an updated approach for computing object keypoint similarity.- The addition of
normalized_instancesuggests an enhancement to incorporate normalized instance similarity for improved candidate selection.The code changes are approved.
|
I'm just seeing this PR and it seems similar in spirit to the "object keypoint" similarity (#1003) that was recently merged, but not correctly documented I'm afraid (I am the author so I am to blame!). |
Hi @getzze, I think part of the problem is that we thought your method was not being hooked up to the GUI at all. Here is where we map the string names of the similarity methods to the actual functions: Lines 494 to 499 in e4bb444 The Lines 886 to 893 in e4bb444 Docs would definitely help, but either way, I think what we're doing here is a bit different as it's basically just normalizing the keypoints by the image size rather than changing how we account for number of keypoints or confidence. I suppose the best thing would be to have something more unified that can (optionally?) use all three factors. For the moment, the easiest might just be to have separate methods as we'll be refactoring a lot of the tracking backend in the coming months anyway as part of our transition to PyTorch. If you're curious or would like to weigh in on the design of the new backend, check out: talmolab/sleap-nn#53 and the current state of the implementation in this source tree. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
sleap/nn/tracking.py (2)
645-646: Ensure the newimg_hwparameter is properly used and documented.The new
img_hwparameter represents the height and width of the image. Make sure it is used correctly within thetrackmethod and consider adding a docstring to document its purpose.
653-653: Update the docstring to include the newimg_hwparameter.The docstring for the
trackmethod should be updated to include a description of the newimg_hwparameter.sleap/nn/inference.py (3)
Line range hint
3911-3914: Passimg_hwto the tracker for consistency.In the
TopDownPredictorandBottomUpPredictorclasses, theimg_hwargument was added to thetrackmethod call to provide the image dimensions to the tracker. For consistency and to ensure the tracker has access to the same context, consider addingimg_hwhere as well:predicted_instances = self.tracker.track( untracked_instances=predicted_instances, img_hw=ex["image"].shape[-2:], # Add this line img=image, t=frame_ind, )
Line range hint
4554-4557: Consider adding tracking functionality for consistency.In the other predictor classes (
TopDownPredictor,BottomUpPredictor,TopDownMultiClassPredictor), ifself.trackeris available, the predicted instances are passed through the tracker before being added to thepredicted_frameslist.For consistency, consider adding similar tracking code here:
if self.tracker: # Set tracks for predicted instances in this frame. predicted_instances = self.tracker.track( untracked_instances=predicted_instances, img_hw=ex["image"].shape[-2:], img=image, t=frame_ind, )
Line range hint
4850-4853: Clarify the purpose of modifyingargs.models.The code block:
if args.models is not None and "movenet" in args.models[0]: args.models = args.models[0]modifies the
args.modelslist if it contains the string "movenet" in the first element. It's unclear why this specific condition is being checked and whyargs.modelsis being reassigned to its first element.Please provide more context on the reasoning behind this code and ensure that it aligns with the intended behavior of the
mainfunction.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- docs/guides/cli.md (1 hunks)
- docs/guides/proofreading.md (1 hunks)
- sleap/nn/inference.py (2 hunks)
- sleap/nn/tracker/components.py (2 hunks)
- sleap/nn/tracking.py (3 hunks)
- tests/nn/test_tracker_components.py (1 hunks)
- tests/nn/test_tracking_integration.py (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- docs/guides/cli.md
- tests/nn/test_tracker_components.py
- tests/nn/test_tracking_integration.py
Additional comments not posted (10)
docs/guides/proofreading.md (2)
53-53: LGTM!The new "normalized_instance" similarity method is a valuable addition that enhances the existing "instance" method by normalizing keypoints by image size. This can make the similarity measure more robust to variations in image resolution or object scale across different datasets.
The method is clearly described and fits well with the existing set of similarity methods.
54-54: Looks good!The new "object_keypoint" similarity method is a great addition that provides a complementary approach to the existing methods. By measuring keypoint-level distances with an exponential decay function and normalizing by the number of visible keypoints, it can effectively capture fine-grained instance similarity while being robust to partial occlusions.
The method is clearly described and adds a valuable new option for measuring instance similarity during tracking.
sleap/nn/tracker/components.py (2)
33-45: LGTM!The
normalized_instance_similarityfunction is a great addition to improve instance matching accuracy by normalizing keypoints based on image dimensions. The implementation is mathematically sound, well-documented, and follows good coding practices.
53-53: Refactoring looks good.The removal of unnecessary line breaks in the distance calculation improves code readability. The core logic remains intact.
sleap/nn/tracking.py (2)
Line range hint
1-1: Skipped reviewingrun_trackerfunction.This function is not part of the changeset. No changes have been made that warrant a review.
660-663: Verify the partial function usage ofnormalized_instance_similarity.The logic for setting the
similarity_functionhas been modified to utilize a partial function ofnormalized_instance_similaritywhen the similarity function is set tonormalized_instance_similarity.This change indicates a more dynamic approach to similarity computation, allowing for the inclusion of image dimensions in the similarity calculations.
Ensure that this partial function is used correctly throughout the codebase and that it provides the expected behavior.
Run the following script to verify the usage of
normalized_instance_similarity:Verification successful
Partial function usage of
normalized_instance_similarityis correctly implemented. The partial functionfactory_normalized_instanceis assigned and used as expected within thesleap/nn/tracking.pyfile, and there are no direct calls tonormalized_instance_similarity, confirming the intended design.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `normalized_instance_similarity` partial function. # Test 1: Search for the partial function assignment. # Expect: Only the assignment within the `track` method. rg --type python -A 5 $'factory_normalized_instance = functools.partial\(normalized_instance_similarity,' # Test 2: Search for usage of the `factory_normalized_instance` variable. # Expect: Usage only within the `track` method after assignment. rg --type python -A 5 $'factory_normalized_instance' # Test 3: Search for direct calls to `normalized_instance_similarity`. # Expect: No direct calls, only usage via the partial function. rg --type python -A 5 $'normalized_instance_similarity\('Length of output: 310
Script:
#!/bin/bash # Description: Verify usage of `normalized_instance_similarity` partial function. # Test 1: Search for the partial function assignment. # Expect: Only the assignment within the `track` method. rg --type py -A 5 $'factory_normalized_instance = functools.partial\(normalized_instance_similarity,' # Test 2: Search for usage of the `factory_normalized_instance` variable. # Expect: Usage only within the `track` method after assignment. rg --type py -A 5 $'factory_normalized_instance' # Test 3: Search for direct calls to `normalized_instance_similarity`. # Expect: No direct calls, only usage via the partial function. rg --type py -A 5 $'normalized_instance_similarity\('Length of output: 1620
sleap/nn/inference.py (4)
2625-2628: LGTM!Passing the image height and width to the tracker via
img_hwis a good change. It provides additional context that the tracking algorithm can utilize.
3268-3271: Looks good!As with the previous change in
TopDownPredictor, passingimg_hwto provide the image dimensions to the tracker is a positive addition.
Line range hint
4872-4875: LGTM!Creating the predictor and setting its tracker based on the command-line arguments is a logical step in the
mainfunction. The code looks good as is.
Line range hint
5020-5023: Nice addition!Supporting running the tracker on existing predictions when no models are provided is a handy feature. It allows users to perform tracking separately from inference, which can be useful in certain workflows.
The code looks good and integrates well with the existing logic in the
mainfunction.
There is that, but the key point of sleap/sleap/nn/tracker/components.py Lines 123 to 128 in e4bb444 You can specify the normalization factor for each node, but what makes more sense is to use the standard error of infering the nodes (say 5 pixels). I didn't find a way to get this error simply though, so the extra parameter (but it's also more flexible like that). Quoting what I wrote in the original PR #1003 :
I should have put some explanation in the docs, really sorry about that... So I think Thanks for linking to the refactoring of tracking, the roadmap looks very exciting! |
Description
Currently, we are facing ID switches in tracking because of very low similarity scores (~/= 0) with instance matching similarity function as it doesn't apply normalization. To address this issue, we add a new
normalized_instance_similarityfunction which normalizes the keypoints based on the image size.Types of changes
Does this address any currently open issues?
#1815
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Bug Fixes
Tests