(3d -> 3c) Add method for multi instance products #1605
Conversation
WalkthroughThe updates to the codebase revolve around enhancing the triangulation process in a GUI application. The Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## liezl/add-method-to-test-instance-grouping #1605 +/- ##
==============================================================================
- Coverage 73.85% 73.81% -0.05%
==============================================================================
Files 135 135
Lines 24617 24636 +19
==============================================================================
+ Hits 18181 18185 +4
- Misses 6436 6451 +15 ☔ View full report in Codecov by Sentry. |
…github.com/talmolab/sleap into liezl/add-method-for-multi-instance-products
roomrys
merged commit ed27d88
into
liezl/add-method-to-test-instance-grouping
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/commands.py (14 hunks)
- tests/gui/test_commands.py (5 hunks)
Additional comments: 18
tests/gui/test_commands.py (6)
- 1069-1074: The method
get_and_verify_enough_instanceshas been updated to acceptframe_idxinstead offrame_indsand no longer takes thetrackparameter. Ensure that all calls to this method have been updated accordingly throughout the codebase.Verification successful
The search with
rghas confirmed the existence of the methodget_and_verify_enough_instanceswithin the codebase. It is present in bothsleap/gui/commands.pyandtests/gui/test_commands.py. The results show multiple calls to this method, which indicates that it is indeed being used.Given this information, we can conclude that the method
get_and_verify_enough_instanceshas been updated as described in the PR objectives and the AI-generated summary. The calls to this method in the test filetest_commands.pyreflect the new method signature, which acceptsframe_idxand does not include thetrackparameter.Based on the results from the
rgcommand, it appears that all calls toget_and_verify_enough_instanceshave been updated to the new signature in the codebase, as no instances of the old signature withframe_indsandtrackparameters were found.Therefore, the review comment has been verified, and the changes to the method signature have been correctly propagated throughout the codebase.
* 1087-1087: The method `get_and_verify_enough_instances` now takes `instances_and_coordinates` as a parameter instead of `instances`. Confirm that all instances of this method call have been updated to pass the correct arguments. * 1262-1264: The method `update_instances` is being called but the result is not being used or checked. Ensure that the expected behavior of this method call is verified, especially since the comment mentions a potential race condition. * 1343-1343: The assertion `assert len(instances) == max_num_instances_in_view ** len(views)` assumes that the number of instances is the product of the maximum number of instances in a view and the number of views. This could be a fragile assumption if the data changes. Verify that this logic is indeed correct and document the reasoning behind this assertion for future maintainers. * 1349-1349: The assertion `assert len(instances_in_view) == max_num_instances_in_view` is made within a loop iterating over cameras. This assumes that each camera view will have the same number of instances, which may not always be the case. Ensure that this assumption is valid and consider handling cases where the number of instances may differ between views. * 1407-1421: The method `_get_instance_grouping` is being tested, but the assertions within the test seem to be based on the assumption that the `selected_instance` is representative of the instances in all views. This may not be a safe assumption if the data varies significantly between views. Ensure that the test is robust and accurately reflects the intended functionality of the method.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old method signature usage with 'frame_inds' and 'track' parameters. ast-grep --lang python --pattern $'get_and_verify_enough_instances($_, $_, $_)' # Search for new method signature usage with 'frame_idx' parameter. ast-grep --lang python --pattern $'get_and_verify_enough_instances($_, $_)'Length of output: 157
Script:
#!/bin/bash # Search for any usage of the method 'get_and_verify_enough_instances' regardless of the number of parameters. ast-grep --lang python --pattern $'get_and_verify_enough_instances($$$)'Length of output: 73
Script:
#!/bin/bash # Use ripgrep to search for the string 'get_and_verify_enough_instances' in the codebase. rg 'get_and_verify_enough_instances'Length of output: 567
sleap/gui/commands.py (12)
- 38-38: Importing
permutationsfromitertoolsis appropriate for generating all possible orderings of instances, which aligns with the PR's objective to handle multiple instances per frame.- 3425-3437: The
do_actionmethod now usesget_instance_grouping_and_reprojected_coordsto obtain the best instance grouping and reprojected coordinates, andupdate_instancesto apply these coordinates. Ensure that the rest of the codebase is updated to handle the new return types and that the methods are properly handling any exceptions that may arise from these operations.- 3504-3508: The
verify_views_and_instancesmethod has been refactored to useget_and_verify_enough_instances. This change should be cross-checked to ensure that all calls toverify_views_and_instancesare updated to accommodate the new logic.- 3525-3552: The
get_and_verify_enough_instancesmethod has been updated to return either a dictionary of instances orFalse. This change in return type must be handled wherever the method is called. Additionally, the exception handling within the method should log the exception for better debugging.try { ... } except Exception as e: + logging.error(f"An error occurred while getting and verifying instances: {e}") raiseVerification successful
The output from the
rgcommand shows that theget_and_verify_enough_instancesmethod insleap/gui/commands.pyis indeed called, and the return value is checked for aFalsecondition. IfinstancesisFalse, the method returnsFalse, which is consistent with the updated return type of the method. This indicates that the new return type is handled correctly where the method is called.Regarding the exception handling, the output does not show any logging within a
try-exceptblock related to theget_and_verify_enough_instancesmethod. However, since the output is truncated and does not show the entire context, we cannot definitively conclude whether there is appropriate exception handling with logging for this method.Based on the information provided, the handling of the new return type of
get_and_verify_enough_instancesis verified, but the exception handling and logging cannot be confirmed without further context.* 3817-3840: The `_get_instance_grouping` method is another new addition that selects the instance grouping with the lowest reprojection error. It's important to ensure that the reprojection error calculation is accurate and that the method is used correctly throughout the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure that the new return type of get_and_verify_enough_instances is handled correctly. ast-grep --lang python --pattern $'get_and_verify_enough_instances($_, $_, $_, $_, $_)'Length of output: 88
Script:
#!/bin/bash # Search for the usage of `get_and_verify_enough_instances` with a flexible number of arguments ast-grep --lang python --pattern $'get_and_verify_enough_instances($$$)'Length of output: 73
Script:
#!/bin/bash # Additionally, check if there's any exception handling that logs errors related to `get_and_verify_enough_instances` rg "get_and_verify_enough_instances" -A 3 -B 3Length of output: 2507
Script:
#!/bin/bash # Check how the return value of `get_and_verify_enough_instances` is handled in `sleap/gui/commands.py` rg "get_and_verify_enough_instances" sleap/gui/commands.py -A 10 -B 10 # Check for exception handling and logging related to `get_and_verify_enough_instances` in `sleap/gui/commands.py` rg "get_and_verify_enough_instances" sleap/gui/commands.py -C 20Length of output: 3793
Verification successful
The
_get_instance_groupingmethod is indeed used within thesleap/gui/commands.pyfile and is also tested intests/gui/test_commands.py. The method's purpose is to select the instance grouping with the lowest reprojection error, which is a critical part of ensuring accurate triangulation in multi-camera setups.The search for "reprojection error" within the codebase reveals multiple occurrences in the
sleap/gui/commands.pyfile, indicating that the calculation and usage of reprojection error are integral to the triangulation process. The method comments suggest that reprojection error is calculated per frame or per instance, and the lowest error is used to determine the best instance grouping.Based on the output from the scripts, it can be concluded that the
_get_instance_groupingmethod is used in the codebase and that reprojection error calculation is a part of the logic within theTriangulateSessionclass.* 3858-3877: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3847-3873]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that _get_instance_grouping is used correctly and that the reprojection error calculation is accurate. ast-grep --lang python --pattern $'_get_instance_grouping($_, $_)'Length of output: 67
Script:
#!/bin/bash # Search for the usage of `_get_instance_grouping` in the codebase. rg "_get_instance_grouping" # Search for the term "reprojection error" to find where and how it is calculated and used. rg "reprojection error"Length of output: 1446
The
_calculate_reprojection_errormethod has been updated to handle different modes of error calculation. Ensure that the method's return values are correctly interpreted and used in the calling context.
- 3899-4049: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3886-3924]
The logic within the loop that calculates reprojection error seems to handle different cases based on the
per_instanceandper_viewflags. Verify that the logic is correct and that the error is being calculated as intended. Also, ensure that the type casting is necessary and correct.
- 3929-3957: The
calculate_error_per_instancemethod is a specialized version of the error calculation. Ensure that it is used in the correct context and that the error calculation is accurate.Verification successful
The method
calculate_error_per_frameis indeed present and used within the codebase. It is referenced in bothtests/gui/test_commands.pyandsleap/gui/commands.py. This confirms that the method is likely being used in the correct context, as it is part of theTriangulateSessionclass and is also being called within test cases, which suggests that its functionality is being verified.* 3961-3990: The `calculate_error_per_view` method is another specialized error calculation method. Verify that it is used correctly and that the error calculation is accurate. * 3995-4022: The `calculate_error_per_frame` method calculates the reprojection error per frame. Ensure that the method is used correctly and that the error calculation is accurate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm that calculate_error_per_instance is used in the correct context. ast-grep --lang python --pattern $'calculate_error_per_instance($_, $_)'Length of output: 73
Script:
#!/bin/bash # Confirm that calculate_error_per_frame is used in the correct context. ast-grep --lang python --pattern $'calculate_error_per_frame($_, $_)'Length of output: 70
Script:
#!/bin/bash # Confirm the usage of calculate_error_per_frame in any context. ast-grep --lang python --pattern $'calculate_error_per_frame'Length of output: 387
Verification successful
The
rgcommand has successfully located the usage of thecalculate_error_per_framemethod within thesleap/gui/commands.pyfile and in thetests/gui/test_commands.pyfile. The method is defined as a static method within theTriangulateSessionclass and is used in the same file, as well as being tested in thetest_commands.pyfile.Based on this information, it can be concluded that the
calculate_error_per_framemethod is indeed used within the codebase, and its usage is tested, which suggests that it is used correctly and that the error calculation is likely accurate, as it is part of the test suite.* 4052-4103: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [4030-4100]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm that calculate_error_per_frame is used in the correct context. ast-grep --lang python --pattern $'calculate_error_per_frame($_, $_)'Length of output: 70
Script:
#!/bin/bash # Search for any usage of the calculate_error_per_frame method in the codebase. ast-grep --lang python --pattern $'calculate_error_per_frame($$$)'Length of output: 67
Script:
#!/bin/bash # Use ripgrep to search for the calculate_error_per_frame method in the codebase. rg 'calculate_error_per_frame' -C 3Length of output: 2127
The
get_products_of_instancesmethod has been updated to handle multi-instance scenarios. Ensure that the method's return values are correctly interpreted and used in the calling context, and that the logic for handling permutations and products of instances is correct.Verification successful
The
rgsearch results indicate that theget_products_of_instancesmethod is indeed used within the codebase. It is referenced in bothtests/gui/test_commands.pyandsleap/gui/commands.py. The specific usage withinsleap/gui/commands.pyis part of the method definition itself, while the usage intests/gui/test_commands.pyis within test cases that likely verify the functionality of theget_products_of_instancesmethod.Given this information, it appears that the method is being called and tested, which suggests that the method's return values and the logic for handling permutations and products of instances are likely being considered in the calling context. However, without seeing the exact implementation and usage within the test cases, we cannot fully verify the correctness of the implementation.
To ensure that the method's return values are correctly interpreted and used in the calling context, and that the logic for handling permutations and products of instances is correct, we would need to examine the test cases in
tests/gui/test_commands.pywhere the method is called.Since the review comment suggests verifying the correctness of the method's usage, and we have found where the method is used in the test cases, we should look at the test cases to confirm that the method is being used correctly.
The output from the
rgcommand provides context around the usage of theget_products_of_instancesmethod within thetests/gui/test_commands.pyfile. The method is called in several test functions, and the assertions following the calls are designed to verify the correctness of the method's behavior.From the provided context, we can see that the test
test_triangulate_session_get_products_of_instanceschecks that the length of theinstancesdictionary returned byget_products_of_instancesis equal to the expected number of hypotheses, which is the maximum number of instances in a view raised to the power of the number of views. This assertion is a direct test of the method's ability to generate all possible products of instances across views.Additionally, the method is used in the
test_triangulate_session_calculate_error_per_frameandtest_triangulate_session_get_instance_groupingtests, where the instances returned byget_products_of_instancesare passed to other methods for further processing.The tests appear to be structured to ensure that the
get_products_of_instancesmethod's return values are correctly interpreted and used in the calling context, and that the logic for handling permutations and products of instances is correct. The assertions and the usage of the method in subsequent calculations provide a form of verification for the method's implementation.Based on the test cases provided, it seems that the
get_products_of_instancesmethod is being used correctly in the context of the tests, which are designed to validate its functionality. Therefore, the review comment's request to ensure that the method's return values are correctly interpreted and used in the calling context, and that the logic for handling permutations and products of instances is correct, appears to be addressed by these tests.* 4389-4405: The `update_instances` method has been refactored to use the new structure of `instances_and_coords`. Ensure that the instances are updated correctly and that the method handles any potential errors during the update process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct usage of get_products_of_instances with the new logic for permutations and products. ast-grep --lang python --pattern $'get_products_of_instances($_, $_, $_)'Length of output: 74
Script:
#!/bin/bash # Search for any usage of get_products_of_instances to see how it is being called in the codebase. ast-grep --lang python --pattern $'get_products_of_instances($$$)'Length of output: 67
Script:
#!/bin/bash # Use rg to search for the string 'get_products_of_instances' to find any occurrence in the codebase. rg 'get_products_of_instances'Length of output: 611
Script:
#!/bin/bash # Examine the test cases in tests/gui/test_commands.py where get_products_of_instances is called. rg 'get_products_of_instances' tests/gui/test_commands.py -C 10Length of output: 2265
| cam = cams_to_include[0] | ||
| video = session[cam] | ||
| lfs = labels.find(video, lf.frame_idx) | ||
| lf = lfs[0] | ||
| lf.instances = [] |
There was a problem hiding this comment.
The code is modifying the instances list of a LabeledFrame object directly by setting it to an empty list. This could potentially lead to unexpected behavior if other parts of the code rely on the original instances. Consider using a method provided by the LabeledFrame class to modify its instances if available.
| ) | ||
| assert isinstance(instances, bool) | ||
| assert not instances | ||
| messages = "".join([rec.message for rec in caplog.records]) | ||
| assert "One or less instances found for frame" in messages | ||
| assert "No Instances found for" in messages |
There was a problem hiding this comment.
The variable messages is concatenated from log records but is not used for any assertions or checks. If this is intended for debugging purposes, it should be removed or commented out for the production code.
| ( | ||
| reprojection_error_per_frame, | ||
| instances_and_coords, | ||
| ) = TriangulateSession.calculate_error_per_frame( |
There was a problem hiding this comment.
The tuple (reprojection_error_per_frame, instances_and_coords) is being assigned but not used within the test. If this is setup for a subsequent test, ensure that the values are actually used; otherwise, consider removing the assignment if it's not necessary.
| def get_instance_grouping_and_reprojected_coords( | ||
| session: RecordingSession, | ||
| instance_hypotheses: Dict[int, Dict[Camcorder, List[Instance]]], | ||
| ): | ||
| """Get instance grouping and reprojected coords with lowest reprojection error. | ||
|
|
||
| Triangulation of all possible groupings needs to be performed... Thus, we return | ||
| the best grouping's triangulation in this step to then be used when updating the | ||
| instance. | ||
|
|
||
| Args: | ||
| session: The `RecordingSession` containing the `Camcorder`s. | ||
| instance_hypotheses: Dict with frame identifier keys (not the frame index) | ||
| and values of another inner dict with `Camcorder` keys and | ||
| `List[Instance]` values. | ||
|
|
||
|
|
||
| Returns: | ||
| best_instances_and_reprojected_coords: Dict with `Camcorder` keys with | ||
| `Tuple[Instance, np.ndarray]` values. | ||
| """ | ||
|
|
||
| # Calculate reprojection error for each instance grouping | ||
| ( | ||
| reprojection_error_per_frame, | ||
| instances_and_coords, | ||
| ) = TriangulateSession.calculate_error_per_frame( | ||
| session=session, | ||
| instances=instance_hypotheses, | ||
| ) | ||
|
|
||
| # Just for type hinting | ||
| reprojection_error_per_frame = cast( | ||
| Dict[int, float], reprojection_error_per_frame | ||
| ) | ||
| instances_and_coords = cast( | ||
| Dict[int, Dict[Camcorder, Iterator[Tuple[Instance, np.ndarray]]]], | ||
| instances_and_coords, | ||
| ) | ||
|
|
||
| # Get instance grouping with lowest reprojection error | ||
| best_instances, frame_id_min_error = TriangulateSession._get_instance_grouping( | ||
| instances=instance_hypotheses, | ||
| reprojection_error_per_frame=reprojection_error_per_frame, | ||
| ) | ||
|
|
||
| # Just for type hinting | ||
| best_instances = cast(Dict[Camcorder, List[Instance]], best_instances) | ||
| instances_and_coords = cast( | ||
| Dict[int, Dict[Camcorder, Iterator[Tuple[Instance, np.ndarray]]]], | ||
| instances_and_coords, | ||
| ) | ||
|
|
||
| # Get the best reprojection | ||
| best_instances_and_reprojected_coords: Dict[ | ||
| Camcorder, Iterator[Tuple[Instance, np.ndarray]] | ||
| ] = instances_and_coords[frame_id_min_error] | ||
|
|
||
| return best_instances_and_reprojected_coords | ||
|
|
There was a problem hiding this comment.
The get_instance_grouping_and_reprojected_coords method is a new addition that performs triangulation to find the best instance grouping. Ensure that the method's error handling is robust, as it performs complex operations that may fail. The use of type casting for hinting is noted, but consider if there's a more direct way to ensure type correctness without casting.
Consider adding error handling within get_instance_grouping_and_reprojected_coords to manage potential failures in the triangulation process.
* Add method and (failing) test to get instance grouping * Rename functions and add a per instance error function * Update tests to accept a dummy instance * Rename last few permutations to products * (3d -> 3c) Add method for multi instance products (#1605)
* Update methods to allow triangulating multiple instances at once * Return instances and coords as a dictionary with cams * Update get_instance_across_views to handle multiple frames * [wip] Update calculate reprojected points to support multiple frames * Finish support for multi-frame reprojection * Remove code to put in next PR * (3b -> 3a) Add method to get single instance permutations (#1586) * Add method to get single instance permutations * Append a dummy instance for missing instances * Correct 'permutations' to 'products' * (3c -> 3b) Add method to test instance grouping (#1599) * (3d -> 3c) Add method for multi instance products (#1605) * (3e -> 3a) Add `InstanceGroup` class (#1618) * Add method to get single instance permutations * Add method and (failing) test to get instance grouping * Append a dummy instance for missing instances * Update tests to accept a dummy instance * Add initial InstanceGroup class * Few extra tests for `InstanceGroup` * Remember instance grouping after testing hypotheses * Use reconsumable iterator for reprojected coords * Only triangulate user instances, add fixture, update tests * Normalize instance reprojection errors * Add `locked`, `_dummy_instance`, `numpy`, and `update_points` * Allow `PredictedPoint`s to be updated as well * Add tests for new attributes and methods * Add methods to create, add, replace, and remove instances * Use PredictedInstance for new/dummy instances * (3f -> 3e) Add `FrameGroup` class (#1665) * (3g -> 3f) Use frame group for triangulation (#1693) * Only use user-`Instance`s for triangulation * Remove unused code * Use `LabeledFrame` class instead of dummy labeled frame * Limit which methods can update `Labels.labeled_frames` * Update cache when Labels. remove_session_video * Remove RecordingSession.instance_groups * [wip] Maintain cached FrameGroup dictionaries * Add unique name (per FrameGroup) requirement for InstanceGroup * Lint * Fix remove_video bug * Add RecordingSession.new_frame_group method * Add TODO comments for later * Fix RecordingSesssion.remove_video bug * Remove FrameGroup._frame_idx_registry class attribute
Description
This method adapts the instance grouping to consider mutliple instances per frame instead of just a single instance per frame.
Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Refactor
TriangulateSessionfor better performance and maintainability.Bug Fixes