(3c -> 3b) Add method to test instance grouping #1599
Conversation
WalkthroughThe update introduces modifications to 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 (
|
|
@coderabbitai review |
![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: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/commands.py (1 hunks)
- tests/gui/test_commands.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/gui/test_commands.py
Additional comments: 1
sleap/gui/commands.py (1)
- 3754-3768: The method
get_instance_groupingseems to only consider the frame with the minimum reprojection error and returns the instances for that frame. This might not be the intended functionality if the goal is to group instances across all frames based on the lowest reprojection error. The method name and docstring suggest a broader scope than what is implemented. Please verify the intended functionality and consider iterating over all frames if necessary.
sleap/gui/commands.py
Outdated
| def calculate_reprojection_per_frame( | ||
| session: RecordingSession, instances: Dict[int, Dict[Camcorder, List[Instance]]] | ||
| ) -> Dict[int, float]: | ||
| """Calculate reprojection error per frame.""" | ||
|
|
||
| reprojection_error_per_frame = {} | ||
|
|
||
| # Triangulate and reproject instance coordinates. | ||
| instances_and_coords: Dict[ | ||
| int, Dict[Camcorder, Iterator[Tuple[Instance, np.ndarray]]] | ||
| ] = TriangulateSession.calculate_reprojected_points( | ||
| session=session, instances=instances | ||
| ) | ||
| for frame_id, instances_in_frame in instances_and_coords.items(): | ||
| frame_error = 0 | ||
| for cam, instances_in_view in instances_in_frame.items(): | ||
| # Compare instance coordinates here | ||
| view_error = 0 | ||
| for inst, inst_coords in instances_in_view: | ||
| node_errors = np.nan_to_num(inst.numpy() - inst_coords) | ||
| instance_error = np.linalg.norm(node_errors) | ||
| view_error += instance_error | ||
| frame_error += view_error | ||
| reprojection_error_per_frame[frame_id] = frame_error | ||
|
|
||
| return reprojection_error_per_frame |
There was a problem hiding this comment.
The method calculate_reprojection_per_frame calculates the reprojection error for each frame, which seems correct. However, there are a few points to consider:
- The use of
np.nan_to_numin line 3790 may mask potential issues with the data. If NaN values are expected and normal, this is fine, but if they indicate a problem with the data or computation, it might be better to handle them explicitly. - The calculation of
instance_errorin line 3791 does not square the difference before taking the norm, which is typical in reprojection error calculations. If the intention is to calculate the Euclidean distance, this is correct. Otherwise, consider squaring the differences. - Ensure that the
np.linalg.normfunction is the correct choice for calculating the instance error. If you're looking for a sum of squared differences, you might need to square thenode_errorsbefore summing. - There is no check for empty
instances_in_view. If there are no instances in view for a particular camera, this could lead to a division by zero or other unexpected behavior when calculating averages or normalizing errors.
…https://github.com/talmolab/sleap into liezl/add-method-to-test-instance-grouping
…https://github.com/talmolab/sleap into liezl/add-method-to-test-instance-grouping
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## liezl/add-method-to-get-single-instance-permutations #1599 +/- ##
========================================================================================
- Coverage 73.83% 73.81% -0.02%
========================================================================================
Files 135 135
Lines 24579 24636 +57
========================================================================================
+ Hits 18147 18185 +38
- Misses 6432 6451 +19 ☔ View full report in Codecov by Sentry. |
…https://github.com/talmolab/sleap into liezl/add-method-to-test-instance-grouping
* Adapt instance grouping from single to multi instance * Add function to get error per view * Replace track-based triangulation with hypothesis-based * Uncomment some commented out code (for testing) * Fix typehinting, add comments * Typehinting
roomrys
merged commit 8267345
into
liezl/add-method-to-get-single-instance-permutations
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/commands.py (11 hunks)
- tests/gui/test_commands.py (3 hunks)
Additional comments: 18
tests/gui/test_commands.py (1)
- 1100-1113: The test setup from lines 1100 to 1104 is modifying the state of
lfby clearing its instances, which could affect other tests iflfis not a local copy. Ensure that this change does not have unintended side effects on other tests that may rely on the original state oflf.sleap/gui/commands.py (17)
- 38-38: The import statement has been correctly updated to include
permutationsfrom theitertoolsmodule, which aligns with the PR objectives of adding new functionality that involves permutations.- 3425-3437: The method
get_instance_grouping_and_reprojected_coordsis being called with appropriate arguments. However, ensure that theinstancesvariable is correctly populated before this call, as it is critical for the method's operation.Verification successful
The verification scripts have provided the following information:
- The
instancesvariable is assigned fromparams["instances"]insleap/gui/commands.pyat lines 2532 and 3424.- There was no output for the search of the
TriangulateSession.get_instance_grouping_and_reprojected_coordsmethod call, which could mean that the pattern did not match due to its structure or that the method call is not present in the codebase.Given the information from the first script, we can confirm that the
instancesvariable is indeed set fromparamsbefore it is used. However, the lack of output for the second script means we cannot confirm the presence of theTriangulateSession.get_instance_grouping_and_reprojected_coordsmethod call with the provided pattern.To ensure that the
instancesvariable is correctly populated before the call toget_instance_grouping_and_reprojected_coords, we need to verify that the assignment at line 3424 insleap/gui/commands.pyis indeed the same context where the method is called. Since the second script did not return results, I will attempt a different approach to verify the method call.
The output from the
ripgrepcommand provides the context around the assignment of theinstancesvariable insleap/gui/commands.py. It shows thatinstancesis assigned fromparams["instances"]in a method that also contains the call toTriangulateSession.get_instance_grouping_and_reprojected_coordswithsessionandinstance_hypothesesas arguments.This confirms that the
instancesvariable is indeed populated before the call toget_instance_grouping_and_reprojected_coords. The method call is within the same block of code as the assignment, ensuring that theinstancesvariable is set appropriately for the method's operation.Based on this information, the review comment is verified as the
instancesvariable is correctly populated before the call toget_instance_grouping_and_reprojected_coords.* 3428-3432: The assignment of `instances_and_reprojected_coords` is done in a tuple unpacking manner, which is a clean and readable approach. Ensure that the method `get_instance_grouping_and_reprojected_coords` returns a tuple that matches this structure. * 3435-3437: The call to `update_instances` is correctly using the result from the previous method call. This is a good example of chaining method calls where the output of one is the input to another. * 3504-3508: The method `get_and_verify_enough_instances` is being called with the correct parameters. However, ensure that the method exists and is implemented to handle the provided arguments.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'instances' variable is set before being used. ast-grep --lang python --pattern $'def $_($_, $_, $_) { $$$ $_ = $_["instances"] $$$ TriangulateSession.get_instance_grouping_and_reprojected_coords($_) $$$ }'Length of output: 515
Script:
#!/bin/bash # Search for the assignment of the 'instances' variable and its usage in the context of TriangulateSession.get_instance_grouping_and_reprojected_coords ast-grep --lang python --pattern $'$_ = $_["instances"] $$$ TriangulateSession.get_instance_grouping_and_reprojected_coords($_)'Length of output: 646
Script:
#!/bin/bash # Search for any assignment to the 'instances' variable ast-grep --lang python --pattern $'$_ = $_["instances"]' # Search for the presence of the 'TriangulateSession.get_instance_grouping_and_reprojected_coords' method call ast-grep --lang python --pattern $'TriangulateSession.get_instance_grouping_and_reprojected_coords($_)'Length of output: 463
Script:
#!/bin/bash # Verify the context of the 'instances' variable assignment and the method call rg "params\[\"instances\"\]" -C 10 sleap/gui/commands.pyLength of output: 3375
Verification successful
The method
get_and_verify_enough_instancesdoes exist in thesleap/gui/commands.pyfile. The verification process has confirmed its presence.* 3525-3552: The method `get_and_verify_enough_instances` has a clear docstring and is using a try-except block to handle potential exceptions. This is good practice for error handling, especially when dealing with external data or computations that may fail. * 3755-3815: The method `get_instance_grouping_and_reprojected_coords` has a well-documented purpose and clear return type. The use of type casting for hinting is acceptable, but ensure that the actual return types from the methods being called match the expected types.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'get_and_verify_enough_instances' method exists and accepts the provided arguments. ast-grep --lang python --pattern $'def get_and_verify_enough_instances($_, $_, $_, $_, $_) { $$$ }'Length of output: 590
Script:
#!/bin/bash # Verify that 'get_and_verify_enough_instances' method exists and accepts parameters. ast-grep --lang python --pattern $'def get_and_verify_enough_instances($$$) { $$$ }'Length of output: 560
Script:
#!/bin/bash # Adjusting the pattern to correctly match a function definition in Python. ast-grep --lang python --pattern $'def get_and_verify_enough_instances($$$)'Length of output: 422
Script:
#!/bin/bash # Using ripgrep to search for the method name directly in the codebase. rg "def get_and_verify_enough_instances"Length of output: 105
Verification successful
The method
calculate_error_per_frameis confirmed to return a tuple consisting of a dictionary with integers as keys and floats as values, and a dictionary with integers as keys and another dictionary as values, which in turn hasCamcorderas keys and an iterator of tuples ofInstanceandnp.ndarrayas values.The method
_get_instance_groupingis confirmed to return a tuple consisting of a dictionary withCamcorderas keys and a list ofInstanceas values, and an integer.These return types match the expected types that are cast in the
get_instance_grouping_and_reprojected_coordsmethod. Therefore, the review comment is verified as the actual return types from the methods being called match the expected types.* 3817-3841: The method `_get_instance_grouping` is private and seems to be used internally within the class. The logic to determine the frame with the minimum error is straightforward and uses Python's built-in `min` function effectively. * 3843-3925: The method `_calculate_reprojection_error` is complex and handles multiple scenarios based on the boolean flags `per_instance` and `per_view`. Ensure that the logic within the loop correctly handles these scenarios and that the return types are consistent with the method's signature. * 3927-3958: The method `calculate_error_per_instance` is a wrapper around `_calculate_reprojection_error` with the `per_instance` flag set to `True`. This is a good use of a wrapper method to provide a specific configuration of a more general method. * 3960-3991: Similarly, `calculate_error_per_view` is another wrapper around `_calculate_reprojection_error`, this time with the `per_view` flag set to `True`. This consistent approach to method design enhances readability and maintainability. * 3993-4023: The method `calculate_error_per_frame` is the default configuration of `_calculate_reprojection_error`, with neither `per_instance` nor `per_view` set to `True`. This method should be carefully tested to ensure that it handles the default case correctly. * 4030-4042: The method `get_products_of_instances` is well-documented and seems to be handling the logic of creating permutations and products of instances across views. Ensure that the logic for handling `require_multiple_views` is implemented as it is mentioned in the docstring but not visible in the provided code.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the return types from 'calculate_error_per_frame' and '_get_instance_grouping' match the expected types. ast-grep --lang python --pattern $'def calculate_error_per_frame($_, $_) { $$$ return ($_,$_,$_) }' ast-grep --lang python --pattern $'def _get_instance_grouping($_, $_) { $$$ return ($_,$_,$_) }'Length of output: 896
Script:
#!/bin/bash # Refine the ast-grep patterns to match the method definitions and their return statements for 'calculate_error_per_frame' and '_get_instance_grouping'. ast-grep --lang python --pattern $'def calculate_error_per_frame($$$) { $$$ return $_ }' ast-grep --lang python --pattern $'def _get_instance_grouping($$$) { $$$ return $_ }'Length of output: 874
Script:
#!/bin/bash # Use ripgrep to search for the method definitions directly within the codebase. rg "def calculate_error_per_frame" rg "def _get_instance_grouping"Length of output: 181
Script:
#!/bin/bash # Now that we have confirmed the existence of the methods, let's extract their definitions and return statements. rg -A 10 "def calculate_error_per_frame" sleap/gui/commands.py rg -A 3 "def _get_instance_grouping" sleap/gui/commands.pyLength of output: 805
Verification successful
The search results confirm that the
require_multiple_viewsparameter is indeed present within theget_products_of_instancesmethod in thesleap/gui/commands.pyfile. The parameter is mentioned in the method's docstring and is also used as an argument within the method, as indicated by the search results.Based on this information, it can be concluded that the method
get_products_of_instancesdoes handle therequire_multiple_viewsparameter as mentioned in the docstring.* 4055-4100: The logic for creating dummy instances and permutations of instances is complex and should be thoroughly tested to ensure that it works as intended, especially the handling of cases where the number of instances is less than the maximum.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'get_products_of_instances' handles 'require_multiple_views' as mentioned in the docstring. ast-grep --lang python --pattern $'def get_products_of_instances($_, $_, $_) { $$$ if $_ { $$$ } $$$ }'Length of output: 618
Script:
#!/bin/bash # Attempting a refined ast-grep command to verify the handling of 'require_multiple_views' in 'get_products_of_instances'. ast-grep --lang python --pattern $'def get_products_of_instances($_, $_, $_, $_) { $$$ if $_ { raise $_ } $$$ }'Length of output: 636
Script:
#!/bin/bash # Simplifying the ast-grep pattern to match the structure of 'get_products_of_instances' and check for an if statement. ast-grep --lang python --pattern $'def get_products_of_instances($$$) { $$$ if $_ { $$$ } $$$ }'Length of output: 604
Script:
#!/bin/bash # Using ripgrep to search for the presence of 'require_multiple_views' within the 'get_products_of_instances' method. rg "def get_products_of_instances" -A 30 sleap/gui/commands.py | rg "require_multiple_views"Length of output: 220
Verification successful
The verification process has confirmed the existence of the
get_products_of_instancesmethod within thesleap/gui/commands.pyfile. However, the previous attempts to verify the logic for creating dummy instances and permutations usingast-grepwere unsuccessful. Thergcommand has now provided evidence that the method does exist, but we still need to verify the internal logic of the method.Given the limitations encountered with
ast-grep, I will proceed withrgto examine the contents of the method to ensure that the logic for handling dummy instances and permutations is implemented correctly.
The output from the
rgcommand has provided the beginning of theget_products_of_instancesmethod, including its signature and part of its docstring. However, the output does not include the full method body, which is necessary to verify the logic for creating dummy instances and permutations.To ensure a thorough verification, I will need to extract more lines following the method signature to review the complete logic within the method.
The additional context provided from the
rgcommand output includes the method signature forget_products_of_instancesand a portion of its implementation. The code snippet shows the creation of a dummy instance and the beginning of the logic for handling permutations of instances across views.However, the output is still incomplete and does not show the entire logic, particularly the handling of cases where the number of instances is less than the maximum, which is crucial for verifying the original review comment.
To complete the verification, I will need to extract the remaining part of the method to review the full implementation of the logic for creating dummy instances and permutations.
The extended output from the
rgcommand has provided the full implementation of theget_products_of_instancesmethod within thesleap/gui/commands.pyfile. The code snippet includes the logic for creating dummy instances and generating permutations of instances across views, which was the focus of the original review comment.Based on the provided code, the method does the following:
- Retrieves all instances across views for a given frame index.
- Determines the maximum number of instances across all views.
- Creates a dummy instance filled with NaN values.
- For each view, if the number of instances is less than the maximum, it appends enough dummy instances to match the maximum number.
- Generates all possible permutations of instances within each view.
- Produces all possible groupings (products) of instances across views.
- Reorganizes these groupings by camera and frame ID to form hypotheses.
The logic for handling cases where the number of instances is less than the maximum is correctly implemented by appending dummy instances to ensure that all lists of instances have the same length before generating permutations.
Therefore, the review comment has been verified, and the logic for creating dummy instances and permutations of instances is correctly implemented as intended in the
get_products_of_instancesmethod.* 4229-4229: The method `_calculate_reprojected_points` is mentioned but not shown in the provided code. Ensure that this method exists and is implemented according to its documentation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the logic for creating dummy instances and permutations is correctly implemented. ast-grep --lang python --pattern $'def get_products_of_instances($_, $_, $_) { $$$ for $_ in $_ { $$$ if $_ < $_ { $$$ } $$$ } $$$ }'Length of output: 509
Script:
#!/bin/bash # Attempt to find the correct structure of the get_products_of_instances method ast-grep --lang python --pattern $'def get_products_of_instances($$$) { $$$ }'Length of output: 548
Script:
#!/bin/bash # Search for any method definitions within the sleap/gui/commands.py file to verify the structure and existence of methods. ast-grep --lang python --pattern $'def $$$($$$) { $$$ }' --file sleap/gui/commands.pyLength of output: 282
Script:
#!/bin/bash # Use ripgrep to search for the method name directly in the sleap/gui/commands.py file. rg "def get_products_of_instances" sleap/gui/commands.pyLength of output: 93
Script:
#!/bin/bash # Extract the content of the get_products_of_instances method to verify the logic for creating dummy instances and permutations. rg -A 30 "def get_products_of_instances" sleap/gui/commands.pyLength of output: 1423
Script:
#!/bin/bash # Extract more lines following the method signature to review the complete logic within the get_products_of_instances method. rg -A 50 "def get_products_of_instances" sleap/gui/commands.pyLength of output: 2242
Script:
#!/bin/bash # Extract the remaining part of the get_products_of_instances method to review the full implementation of the logic for creating dummy instances and permutations. rg -A 70 "def get_products_of_instances" sleap/gui/commands.pyLength of output: 3208
Verification successful
The method
_calculate_reprojected_pointsdoes indeed exist within thesleap/gui/commands.pyfile, as confirmed by the successful search usingripgrep.* 4376-4376: The comment about reordering instances and reprojected coordinates suggests that there is a method `group_instances_and_coords` being used. Ensure that this method is implemented and correctly groups the instances as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that '_calculate_reprojected_points' exists and is implemented as documented. ast-grep --lang python --pattern $'def _calculate_reprojected_points($_, $_) { $$$ }'Length of output: 562
Script:
#!/bin/bash # Correcting the syntax and trying to verify the existence of '_calculate_reprojected_points' again. ast-grep --lang python --pattern $'def _calculate_reprojected_points($$$) { $$$ }'Length of output: 556
Script:
#!/bin/bash # Simplifying the pattern to search for the method name '_calculate_reprojected_points'. ast-grep --lang python --pattern $'def _calculate_reprojected_points($$$)'Length of output: 420
Script:
#!/bin/bash # Attempting a more general pattern to search for the method name '_calculate_reprojected_points'. ast-grep --lang python --pattern $'def _calculate_reprojected_points'Length of output: 415
Script:
#!/bin/bash # Switching to ripgrep to search for the method name '_calculate_reprojected_points' in the codebase. rg --type py 'def _calculate_reprojected_points'Length of output: 111
Verification successful
The method
group_instances_and_coordsis indeed implemented in thesleap/gui/commands.pyfile. This confirms the existence of the method referenced in the comment.* 4389-4405: The method `update_instances` is straightforward and updates instance coordinates with reprojected coordinates. This is a critical operation and should be tested to ensure that the update is performed correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'group_instances_and_coords' is implemented and groups instances correctly. ast-grep --lang python --pattern $'def group_instances_and_coords($_, $_) { $$$ }'Length of output: 556
Script:
#!/bin/bash # Adjusting the ast-grep pattern to correctly match the method definition in Python. ast-grep --lang python --pattern $'def group_instances_and_coords($$$) { $$$ }'Length of output: 550
Script:
#!/bin/bash # Attempting a different approach to match the method definition using ast-grep. ast-grep --lang python --pattern $'def group_instances_and_coords($_, $_) { $$$ }'Length of output: 556
Script:
#!/bin/bash # Using ripgrep to search for the method name directly in the codebase. rg "def group_instances_and_coords"Length of output: 95
| session=session, frame_idx=lf.frame_idx | ||
| ) | ||
| instances_in_frame = instances[lf.frame_idx] | ||
| instances_in_frame = instances[0] | ||
| assert ( | ||
| len(instances_in_frame) == 6 | ||
| ) # Some views don't have an instance at this track | ||
| len(instances_in_frame) == 8 | ||
| ) # All views should have same number of instances (padded with dummy instance(s)) |
There was a problem hiding this comment.
The assertion on line 1073 assumes that all views should have the same number of instances, which may not be true if there are different numbers of instances per view. This could lead to a false positive if the number of instances is coincidentally the same across views that should not have the same count. Consider verifying the expected number of instances for each specific view.
| frame_idx=lf.frame_idx, | ||
| cams_to_include=cams_to_include, | ||
| track=track, | ||
| ) | ||
| instances_in_frame = instances[lf.frame_idx] | ||
| assert len(instances_in_frame) == len(cams_to_include) |
There was a problem hiding this comment.
The variable instances_in_frame is indexed using lf.frame_idx on line 1089, but it should be indexed using 0 as per the changes in the code. This is because instances is now a list, not a dictionary keyed by frame indices.
- instances_in_frame = instances[lf.frame_idx]
+ instances_in_frame = instances[0]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.
| frame_idx=lf.frame_idx, | |
| cams_to_include=cams_to_include, | |
| track=track, | |
| ) | |
| instances_in_frame = instances[lf.frame_idx] | |
| assert len(instances_in_frame) == len(cams_to_include) | |
| frame_idx=lf.frame_idx, | |
| cams_to_include=cams_to_include, | |
| ) | |
| instances_in_frame = instances[0] | |
| assert len(instances_in_frame) == len(cams_to_include) |
| TriangulateSession.update_instances( | ||
| instances_and_coords=instances_and_coordinates[0] | ||
| ) |
There was a problem hiding this comment.
The method TriangulateSession.update_instances is being tested, but there is no assertion or verification of the outcome. This makes it difficult to ensure that the method is functioning correctly. Consider adding assertions to verify that the instances have been updated as expected.
| assert len(instances) == max_num_instances_in_view ** len(views) | ||
|
|
||
| for frame_id in instances: | ||
| instances_in_frame = instances[frame_id] | ||
| for cam in instances_in_frame: | ||
| instances_in_view = instances_in_frame[cam] | ||
| assert len(instances_in_view) == 1 | ||
| assert len(instances_in_view) == max_num_instances_in_view |
There was a problem hiding this comment.
The assertion on line 1343 assumes that the number of instances is equal to the maximum number of instances in view raised to the power of the number of views. This could be incorrect if the number of instances varies per view. The test should verify the expected number of permutations of instances across views, not just the mathematical product of instances and views.
|
|
||
| def test_triangulate_session_calculate_error_per_frame( | ||
| multiview_min_session_labels: Labels, | ||
| ): | ||
| """Test `TriangulateSession.calculate_error_per_frame`.""" | ||
|
|
||
| labels = multiview_min_session_labels | ||
| session = labels.sessions[0] | ||
| lf = labels.labeled_frames[0] | ||
|
|
||
| instances = TriangulateSession.get_products_of_instances( | ||
| session=session, | ||
| frame_idx=lf.frame_idx, | ||
| ) | ||
|
|
||
| ( | ||
| reprojection_error_per_frame, | ||
| instances_and_coords, | ||
| ) = TriangulateSession.calculate_error_per_frame( | ||
| session=session, instances=instances | ||
| ) | ||
|
|
||
| for frame_id in instances.keys(): | ||
| assert frame_id in reprojection_error_per_frame | ||
| assert isinstance(reprojection_error_per_frame[frame_id], float) | ||
|
|
||
|
|
||
| def test_triangulate_session_get_instance_grouping( | ||
| multiview_min_session_labels: Labels, | ||
| ): | ||
| """Test `TriangulateSession._get_instance_grouping`.""" | ||
|
|
||
| labels = multiview_min_session_labels | ||
| session = labels.sessions[0] | ||
| lf = labels.labeled_frames[0] | ||
| selected_instance = lf.instances[0] | ||
|
|
||
| instances = TriangulateSession.get_products_of_instances( | ||
| session=session, | ||
| frame_idx=lf.frame_idx, | ||
| ) | ||
|
|
||
| ( | ||
| reprojection_error_per_frame, | ||
| instances_and_coords, | ||
| ) = TriangulateSession.calculate_error_per_frame( | ||
| session=session, instances=instances | ||
| ) | ||
|
|
||
| best_instances, frame_id_min_error = TriangulateSession._get_instance_grouping( | ||
| instances=instances, reprojection_error_per_frame=reprojection_error_per_frame | ||
| ) | ||
| assert len(best_instances) == len(session.camera_cluster) | ||
| for instances_in_view in best_instances.values(): | ||
| tracks_in_view = set( | ||
| [inst.track if inst is not None else "None" for inst in instances_in_view] | ||
| ) | ||
| assert len(tracks_in_view) == len(instances_in_view) | ||
| for inst in instances_in_view: | ||
| try: | ||
| assert inst.frame_idx == selected_instance.frame_idx | ||
| except: | ||
| assert inst.frame is None | ||
| assert inst.track is None |
There was a problem hiding this comment.
The tests for TriangulateSession.calculate_error_per_frame and TriangulateSession._get_instance_grouping do not include assertions to verify the correctness of the reprojection error calculations or the instance grouping. Without these assertions, the tests cannot guarantee the correctness of the methods under test.
|
|
||
| # TODO(LM): I think we will need a reconsumable iterator here. |
There was a problem hiding this comment.
The comment indicates a potential future change regarding the use of a reconsumable iterator. This should be noted for future development and possibly tracked as a technical debt item.
Would you like me to open a GitHub issue to track the need for a reconsumable iterator as indicated by the TODO comment?
* 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 PR adds a method to test and retrieve the "correct" instance grouping. Here "correct" means the instance grouping with the lowest total reprojection error across all frames.
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
Tests