(3b -> 3a) Add method to get single instance permutations #1586
Conversation
WalkthroughThe codebase has been updated to include several changes. 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: 6
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/commands.py (3 hunks)
- tests/gui/test_commands.py (2 hunks)
Additional comments: 1
sleap/gui/commands.py (1)
- 3754-3814: The
get_permutations_of_instancesmethod seems to be well implemented. It's good to see that type hints are used for function arguments and the return type, which improves readability and maintainability. The docstring is also comprehensive and provides a clear explanation of the method's purpose, arguments, and return value. However, it's important to ensure that theselected_instanceis indeed part of thesessionand that theframe_idxis within the valid range for the session. If these conditions are not met, the function may not behave as expected.
sleap/gui/commands.py
Outdated
| cam_selected = session.get_camera(selected_instance.video) | ||
| cam_selected = cast(Camcorder, cam_selected) # Could be None if not in session |
There was a problem hiding this comment.
The cast function is used to ensure that cam_selected is of type Camcorder. However, the comment indicates that cam_selected could be None if it's not in the session. If cam_selected is None, the cast function will not raise an error but will simply return None. This could lead to unexpected behavior later in the code. Consider adding a check to ensure that cam_selected is not None before proceeding.
if cam_selected is None:
raise ValueError("The selected instance's video is not in the session.")
sleap/gui/commands.py
Outdated
| # Reorganize permutaions by cam and add selected instance to each permutation | ||
| instances: Dict[int, Dict[Camcorder, List[Instance]]] = {} | ||
| for frame_id, perm in enumerate(permutated_instances): | ||
| instances[frame_id] = {cam_selected: [selected_instance]} | ||
| instances[frame_id].update( | ||
| { | ||
| cam: [inst] | ||
| for cam, inst in zip(instances_excluding_selected.keys(), perm) | ||
| } | ||
| ) | ||
|
|
||
| return instances # Expect <num instances in other views>! frames |
There was a problem hiding this comment.
The code is creating permutations of instances and then reorganizing them by camera. This is a complex operation that could be simplified by using a more efficient data structure or algorithm. Consider refactoring this section of the code to improve readability and performance.
There was a problem hiding this comment.
What data structure would you recommend?
There was a problem hiding this comment.
@roomrys, one approach could be to use a data structure like a Multi-Dimensional Array or a Dictionary of Dictionaries, where the first key is the camera and the second key is the instance. This could potentially simplify the process of reorganizing the permutations by camera and instance.
However, the choice of data structure largely depends on the specific requirements of your application and the constraints you're working under. For example, if memory usage is a concern, you might want to choose a more space-efficient data structure. If speed is a concern, you might want to choose a data structure that allows for faster access or manipulation of data.
In terms of improving the algorithm, you could consider using a different approach to generate the permutations. For example, instead of generating all permutations and then filtering out the ones that don't meet your criteria, you could generate only the permutations that do meet your criteria. This could potentially improve performance by reducing the number of permutations that need to be generated and processed.
I hope this helps! If you have any other questions, feel free to ask.
There was a problem hiding this comment.
So the reason I need to reorganize is because itertools.product yields a list of tuples where each tuple in the list is a possible frame and each element in the tuple is the instance at camera/view. I am using a dictionary of dictionaries as soon as I can. Do you think it would be faster to not use itertools.product and try to create permutations on my own?
There was a problem hiding this comment.
@roomrys, the itertools.product function is a powerful tool for generating Cartesian products, and it's implemented in C in the Python standard library, which makes it quite efficient. However, if you're finding that a significant amount of time is being spent reorganizing the output of itertools.product into the format you need, it might be worth considering an alternative approach.
One possibility could be to write a custom generator function that yields permutations in the format you need directly. This could potentially save time by avoiding the need to reorganize the data after generating the permutations. However, writing such a function could be more complex and error-prone than using itertools.product, and it might not necessarily be faster, depending on the specifics of your implementation and the data you're working with.
Another approach could be to use a different library or function that generates permutations in a more suitable format. For example, the numpy library provides a meshgrid function that can generate multi-dimensional grids, which might be more suitable for your needs than itertools.product.
Ultimately, the best approach depends on the specifics of your use case and the constraints you're working under. It might be worth profiling your code to see where the bottlenecks are and trying out different approaches to see which one works best in practice.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## liezl/add-method-to-match-instances-across-views #1586 +/- ##
====================================================================================
+ Coverage 73.76% 73.81% +0.05%
====================================================================================
Files 135 135
Lines 24557 24636 +79
====================================================================================
+ Hits 18115 18186 +71
- Misses 6442 6450 +8 ☔ View full report in Codecov by Sentry. |
…tps://github.com/talmolab/sleap into liezl/add-method-to-get-single-instance-permutations
* 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)
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 (12 hunks)
- tests/gui/test_commands.py (3 hunks)
Additional comments: 15
sleap/gui/commands.py (15)
- 38-38: Imports of
permutationsandproductfromitertoolsare added.- 3425-3437: The
do_actionmethod now usesget_instance_grouping_and_reprojected_coordsandupdate_instancesfromTriangulateSession.- 3504-3508: The
verify_views_and_instancesmethod now callsget_and_verify_enough_instancesfromTriangulateSession.- 3756-3814: The
get_instance_grouping_and_reprojected_coordsstatic method is added toTriangulateSession. It calculates reprojection error and selects the best instance grouping based on the lowest error.- 3817-3840: The
_get_instance_groupingstatic method is added toTriangulateSession. It retrieves the instance grouping with the lowest reprojection error.- 3843-3924: The
_calculate_reprojection_errorstatic method is added toTriangulateSession. It calculates reprojection error per frame, per instance, or per view.- 3927-3957: The
calculate_error_per_instancestatic method is added toTriangulateSession. It calculates reprojection error per instance.- 3960-3989: The
calculate_error_per_viewstatic method is added toTriangulateSession. It calculates reprojection error per view.- 3993-4021: The
calculate_error_per_framestatic method is added toTriangulateSession. It calculates reprojection error per frame.- 4025-4101: The
get_products_of_instancesstatic method is added toTriangulateSession. It generates all possible combinations of instances across views.- 4156-4158: The
get_instances_matricesstatic method is updated to castcams_orderedto a list. Ensure that this cast is safe andcams_orderedcannot beNoneat this point.Verification successful
The verification process has confirmed that
cams_orderedis always a list when it is used within theget_instances_matricesmethod. The method ensures thatcams_orderedis assigned a list of cameras, either frominstances_in_frameif no session is provided, or fromsession.camerasif a session is available. Therefore, the cast to a list in the code is safe, and the concern raised in the review comment has been addressed.* 4229-4229: The `_calculate_reprojected_points` static method is added to `TriangulateSession`. It triangulates and reprojects instance coordinates. * 4277-4281: The `group_instances_and_coords` static method is added to `TriangulateSession`. It groups instances and reprojected coordinates by frame and view. * 4376-4376: The `group_instances_and_coords` method is used to reorder instances and reprojected coordinates. * 4389-4405: The `update_instances` static method is added to `TriangulateSession`. It updates the coordinates of `Instance` objects.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that `cams_ordered` is always a list when passed to `get_instances_matrices`. ast-grep --lang python --pattern $'get_instances_matrices($_, $_, $_)'Length of output: 71
Script:
#!/bin/bash # Search for calls to `get_instances_matrices` and check the initialization of `cams_ordered`. rg "get_instances_matrices"Length of output: 371
Script:
#!/bin/bash # Inspect the code around the call to `get_instances_matrices` in `sleap/gui/commands.py` to verify the initialization of `cams_ordered`. rg -C 10 "get_instances_matrices" sleap/gui/commands.pyLength of output: 1859
Script:
#!/bin/bash # Inspect the method definition of `get_instances_matrices` in `sleap/gui/commands.py` to verify the initialization of `cams_ordered`. rg -C 30 "def get_instances_matrices" sleap/gui/commands.pyLength of output: 2772
Script:
#!/bin/bash # Inspect the rest of the `get_instances_matrices` method to see how `cams_ordered` is assigned a value from the `session`. rg -C 50 "def get_instances_matrices" sleap/gui/commands.pyLength of output: 4322
| 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 method get_and_verify_enough_instances is expected to return a dictionary, but the test seems to assert that it returns a boolean when not enough instances are found. This is inconsistent with the method's expected behavior and could lead to issues when the method is used elsewhere in the code.
- assert isinstance(instances, bool)
- assert not instances
+ assert isinstance(instances, dict)
+ # Additional assertions to check the contents of the dictionary may be necessaryCommittable 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.
| 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)) | |
| session=session, frame_idx=lf.frame_idx | |
| ) | |
| instances_in_frame = instances[0] | |
| assert isinstance(instances, dict) | |
| # Additional assertions to check the contents of the dictionary may be necessary | |
| assert ( | |
| len(instances_in_frame) == 8 | |
| ) # All views should have same number of instances (padded with dummy instance(s)) |
| TriangulateSession.update_instances( | ||
| instances_and_coords=instances_and_coordinates[0] | ||
| ) |
There was a problem hiding this comment.
The call to TriangulateSession.update_instances is not being tested for any specific outcome. There should be assertions to verify that the instances are updated correctly.
Consider adding assertions to check that the instances have been updated as expected.
|
|
||
|
|
||
| def test_triangulate_session_get_products_of_instances( | ||
| multiview_min_session_labels: Labels, | ||
| ): | ||
| """Test `TriangulateSession.get_products_of_instances`.""" | ||
|
|
||
| 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, | ||
| ) | ||
|
|
||
| views = TriangulateSession.get_all_views_at_frame(session, lf.frame_idx) | ||
| max_num_instances_in_view = max([len(instances) for instances in views.values()]) | ||
| 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) == max_num_instances_in_view | ||
| for inst in instances_in_view: | ||
| try: | ||
| assert inst.frame_idx == selected_instance.frame_idx | ||
| assert inst.video == session[cam] | ||
| except: | ||
| assert inst.frame is None | ||
| assert inst.video is None | ||
|
|
There was a problem hiding this comment.
The test test_triangulate_session_get_products_of_instances does not seem to include assertions to verify the correctness of the permutations generated by get_products_of_instances. It only checks the number of instances and their frame indices.
Add assertions to verify that the permutations of instances are generated correctly, not just their count.
| 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.
In the test test_triangulate_session_get_instance_grouping, there is an assertion that the length of best_instances is equal to the length of session.camera_cluster. However, there is no assertion to check if frame_id_min_error is correctly identifying the frame with the minimum error.
Add an assertion to ensure that frame_id_min_error correctly identifies the frame with the minimum reprojection error.
| frame_idx: int, | ||
| context: Optional[CommandContext] = None, | ||
| cams_to_include: Optional[List[Camcorder]] = None, | ||
| track: Union[Track, int] = -1, | ||
| show_dialog: bool = True, | ||
| ) -> Union[Dict[int, Dict[Camcorder, List[Instance]]], bool]: | ||
| """Get all instances accross views at this frame index. | ||
| """Get all instances accross views at this frame index (and products of instances). | ||
|
|
||
| If not enough `Instance`s are available at this frame index, then return False. | ||
|
|
||
| Args: | ||
| session: The `RecordingSession` containing the `Camcorder`s. | ||
| frame_inds: List of frame indices to get instances from (0-indexed). | ||
| frame_idx: Frame index to get instances from (0-indexed). | ||
| context: The optional command context used to display a dialog. | ||
| cams_to_include: List of `Camcorder`s to include. Default is all. | ||
| track: `Track` object used to find instances accross views. Default is -1 | ||
| which finds all instances regardless of track. | ||
| show_dialog: If True, then show a warning dialog. Default is True. | ||
|
|
||
| Returns: | ||
| Dict with frame identifier keys (does not necessarily need to be the frame | ||
| index) and values of another inner dict with `Camcorder` keys and | ||
| `List[Instance]` values if enough instances are found, False otherwise. | ||
| Dict with frame identifier keys (not the frame index) and values of another | ||
| inner dict with `Camcorder` keys and `List[Instance]` values if enough | ||
| instances are found, False otherwise. | ||
| """ | ||
|
|
||
| try: | ||
| instances: Dict[ | ||
| int, Dict[Camcorder, List[Instance]] | ||
| ] = TriangulateSession.get_instances_across_views_multiple_frames( | ||
| ] = TriangulateSession.get_products_of_instances( |
There was a problem hiding this comment.
The get_and_verify_enough_instances method is added to TriangulateSession. It includes a try-except block but does not handle the exception other than re-raising it.
- except Exception as e:
+ except Exception as e:
+ # Handle the exception or log it before re-raising
+ # Example: logging.error("Error while getting and verifying instances: %s", e)
+ raiseCommittable 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: int, | |
| context: Optional[CommandContext] = None, | |
| cams_to_include: Optional[List[Camcorder]] = None, | |
| track: Union[Track, int] = -1, | |
| show_dialog: bool = True, | |
| ) -> Union[Dict[int, Dict[Camcorder, List[Instance]]], bool]: | |
| """Get all instances accross views at this frame index. | |
| """Get all instances accross views at this frame index (and products of instances). | |
| If not enough `Instance`s are available at this frame index, then return False. | |
| Args: | |
| session: The `RecordingSession` containing the `Camcorder`s. | |
| frame_inds: List of frame indices to get instances from (0-indexed). | |
| frame_idx: Frame index to get instances from (0-indexed). | |
| context: The optional command context used to display a dialog. | |
| cams_to_include: List of `Camcorder`s to include. Default is all. | |
| track: `Track` object used to find instances accross views. Default is -1 | |
| which finds all instances regardless of track. | |
| show_dialog: If True, then show a warning dialog. Default is True. | |
| Returns: | |
| Dict with frame identifier keys (does not necessarily need to be the frame | |
| index) and values of another inner dict with `Camcorder` keys and | |
| `List[Instance]` values if enough instances are found, False otherwise. | |
| Dict with frame identifier keys (not the frame index) and values of another | |
| inner dict with `Camcorder` keys and `List[Instance]` values if enough | |
| instances are found, False otherwise. | |
| """ | |
| try: | |
| instances: Dict[ | |
| int, Dict[Camcorder, List[Instance]] | |
| ] = TriangulateSession.get_instances_across_views_multiple_frames( | |
| ] = TriangulateSession.get_products_of_instances( | |
| except Exception as e: | |
| # Handle the exception or log it before re-raising | |
| # Example: logging.error("Error while getting and verifying instances: %s", e) | |
| raise |
| insts_and_coords_in_frame: Dict[ | ||
| Camcorder, Iterator[Tuple[Instance, np.ndarray]] | ||
| ] = {} | ||
| for cam_idx, cam in enumerate(cams_ordered): | ||
| instances_in_frame_ordered: List[Instance] = instances_in_frame[ | ||
| cam | ||
| ] # Reorder by cam to match coordinates, len(T) | ||
| insts_coords_in_frame: np.ndarray = insts_coords_list[frame_idx][ | ||
| cam_idx | ||
| ] # len(T) of N x 2 | ||
| insts_and_coords_in_frame[cam]: Tuple[Instance, np.ndarray] = zip( | ||
|
|
||
| # TODO(LM): I think we will need a reconsumable iterator here. | ||
| insts_and_coords_in_frame[cam] = zip( |
There was a problem hiding this comment.
The group_instances_and_coords method contains a TODO comment about needing a reconsumable iterator.
Would you like me to help with implementing a reconsumable iterator or should we open a GitHub issue to track this task?
roomrys
merged commit 443d410
into
liezl/add-method-to-match-instances-across-views
* 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
roomrys
deleted the
liezl/add-method-to-get-single-instance-permutations
branch
Description
This PR adds a method to get single instance permutations across views (as hypotheses for the selected instance).
Design Decision: Missing
Instances(To Be Implemented)How do we handle the case when the selected instance is not in all views? We would want to create permutations that allow for empty instances in those views. We could essentially create sub-permutations by removing 1 view, then 2 views, and so on until there is only 2 views in the hypothesized frame. We would still need some error threshold to determine whether there is only a single selected instance across views. This will definitely increase the cost and complicate things.Update: We opted to add dummy instances for hypothesis testing. Furthermore, higher up in the stacked PRs (#1618), we add an
InstanceGroupclass to hold allInstances which are the sameInstancebut across views. Even higher up in the stacked PRs (#1165), we add aFrameGroupclass which holds allLabeledFrames that are the sameLabeledFrameacross views.FrameGroupalso manages allInstanceGroups for that temporalframe_idx, including hypothesis generation for "unlocked"Instances (i.e.Instances that are not in alockedInstanceGroup, not assigned/verified by the user).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