Prioritize user instances when coping from prior frame #1658
Conversation
WalkthroughThe recent update to the GUI application extended the logic for setting the copy instance to consider the type of instance and prioritize user instances over predicted instances. It introduced new static methods in 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 @@
## develop #1658 +/- ##
===========================================
+ Coverage 73.38% 73.41% +0.03%
===========================================
Files 134 134
Lines 24064 24083 +19
===========================================
+ Hits 17659 17681 +22
+ Misses 6405 6402 -3 ☔ View full report in Codecov by Sentry. |
|
@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 (2 hunks)
- tests/gui/test_commands.py (3 hunks)
Additional comments: 5
tests/gui/test_commands.py (4)
21-21: The addition of the
Pointclass to the import statements is noted. Ensure that this class is used in the new test functions as expected.619-662: The new test function
test_CopyPriorFramePreviousUsercorrectly sets up a command context and asserts the behavior of copying a user instance from a previous frame. The assertions check that the new instance matches the user instance from the previous frame in terms of video, points, and track.664-707: The test function
test_CopyPriorFramePreviousUser2is designed to verify the copying of a user instance in a scenario where the current frame has no instances. The assertions confirm that the user instance in the current frame matches the user instance from the previous frame.710-759: The test function
test_CopyPriorFrameCurrentUserchecks the copying of a user instance in the current frame when the current frame has an equal or greater number of instances than the previous frame. The assertions ensure that the newly added instance matches the existing user instance in the current frame.sleap/gui/commands.py (1)
- 3022-3038: The new static method
find_last_user_instancehas been added to theAddInstanceclass. This method is used to find the last user instance in a given frame, which is a part of the logic for copying instances. The implementation looks correct and uses theuser_instancesproperty of theLabeledFrameto get the last user instance if available.
sleap/gui/commands.py
Outdated
| prev_frame = context.labels.find( | ||
| context.state["video"], prev_idx, return_new=True | ||
| )[0].instances | ||
| )[0] | ||
| prev_instances = prev_frame.instances | ||
| if len(prev_instances) > len(context.state["labeled_frame"].instances): | ||
| # If more instances in previous frame than current, then use the | ||
| # first unmatched instance. | ||
| copy_instance = prev_instances[ | ||
| len(context.state["labeled_frame"].instances) | ||
| ] | ||
|
|
||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in previous frame, if present | ||
| user_instance = AddInstance.find_last_user_instance(prev_frame) | ||
| if user_instance is not None: | ||
| copy_instance = user_instance | ||
|
|
||
| from_prev_frame = True | ||
| elif init_method == "best" and ( | ||
| context.state["labeled_frame"].instances | ||
| ): | ||
| # Otherwise, if there are already instances in current frame, | ||
| # copy the points from the last instance added to frame. | ||
| copy_instance = context.state["labeled_frame"].instances[-1] | ||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in current frame | ||
| user_instance = AddInstance.find_last_user_instance( | ||
| context.state["labeled_frame"] | ||
| ) | ||
| if user_instance is not None: | ||
| copy_instance = user_instance | ||
|
|
||
| elif len(prev_instances): | ||
| # Otherwise use the last instance added to previous frame. | ||
| copy_instance = prev_instances[-1] | ||
|
|
||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in previous frame, if present | ||
| user_instance = AddInstance.find_last_user_instance(prev_frame) | ||
| if user_instance is not None: | ||
| copy_instance = user_instance | ||
|
|
There was a problem hiding this comment.
The method find_instance_to_copy_from has been updated to include calls to find_last_user_instance. This change ensures that when copying instances from a prior frame, the last user instance is prioritized over predicted instances. The logic appears to be correct, and the use of isinstance checks ensures that the method is robust to different instance types. However, there is a repeated pattern that could be refactored to reduce code duplication.
- if isinstance(copy_instance, PredictedInstance):
- # Set copy instance to last user instance in previous frame, if present
- user_instance = AddInstance.find_last_user_instance(prev_frame)
- if user_instance is not None:
- copy_instance = user_instance
+ # Refactor repeated logic into a method
+ copy_instance = self._replace_with_user_instance_if_needed(copy_instance, prev_frame)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.
| prev_frame = context.labels.find( | |
| context.state["video"], prev_idx, return_new=True | |
| )[0].instances | |
| )[0] | |
| prev_instances = prev_frame.instances | |
| if len(prev_instances) > len(context.state["labeled_frame"].instances): | |
| # If more instances in previous frame than current, then use the | |
| # first unmatched instance. | |
| copy_instance = prev_instances[ | |
| len(context.state["labeled_frame"].instances) | |
| ] | |
| if isinstance(copy_instance, PredictedInstance): | |
| # Set copy instance to last user instance in previous frame, if present | |
| user_instance = AddInstance.find_last_user_instance(prev_frame) | |
| if user_instance is not None: | |
| copy_instance = user_instance | |
| from_prev_frame = True | |
| elif init_method == "best" and ( | |
| context.state["labeled_frame"].instances | |
| ): | |
| # Otherwise, if there are already instances in current frame, | |
| # copy the points from the last instance added to frame. | |
| copy_instance = context.state["labeled_frame"].instances[-1] | |
| if isinstance(copy_instance, PredictedInstance): | |
| # Set copy instance to last user instance in current frame | |
| user_instance = AddInstance.find_last_user_instance( | |
| context.state["labeled_frame"] | |
| ) | |
| if user_instance is not None: | |
| copy_instance = user_instance | |
| elif len(prev_instances): | |
| # Otherwise use the last instance added to previous frame. | |
| copy_instance = prev_instances[-1] | |
| if isinstance(copy_instance, PredictedInstance): | |
| # Set copy instance to last user instance in previous frame, if present | |
| user_instance = AddInstance.find_last_user_instance(prev_frame) | |
| if user_instance is not None: | |
| copy_instance = user_instance | |
| prev_frame = context.labels.find( | |
| context.state["video"], prev_idx, return_new=True | |
| )[0] | |
| prev_instances = prev_frame.instances | |
| if len(prev_instances) > len(context.state["labeled_frame"].instances): | |
| # If more instances in previous frame than current, then use the | |
| # first unmatched instance. | |
| copy_instance = prev_instances[ | |
| len(context.state["labeled_frame"].instances) | |
| ] | |
| copy_instance = self._replace_with_user_instance_if_needed(copy_instance, prev_frame) | |
| from_prev_frame = True | |
| elif init_method == "best" and ( | |
| context.state["labeled_frame"].instances | |
| ): | |
| # Otherwise, if there are already instances in current frame, | |
| # copy the points from the last instance added to frame. | |
| copy_instance = context.state["labeled_frame"].instances[-1] | |
| copy_instance = self._replace_with_user_instance_if_needed(copy_instance, context.state["labeled_frame"]) | |
| elif len(prev_instances): | |
| # Otherwise use the last instance added to previous frame. | |
| copy_instance = prev_instances[-1] | |
| copy_instance = self._replace_with_user_instance_if_needed(copy_instance, prev_frame) | |
There was a problem hiding this comment.
I agree, the same pattern is used over an over, maybe we can just include that into the new method you created.
There was a problem hiding this comment.
@roomrys Absolutely, the repeated logic can be encapsulated within the find_last_user_instance method to streamline the code. This will ensure that the method not only finds the last user instance but also replaces the predicted instance with the user instance when appropriate. I'll adjust the method accordingly.
roomrys
changed the title
There was a problem hiding this comment.
Very thorough tests and good code re-use. The only thing that really needs attention is the first case where
len(prev_instances) > len(context.state["labeled_frame"].instances)I don't think that the logic of just taking the last user instance is what was intended here. See the suggestion for this section of code. Everything else is more of a suggestion. Good work 👍🏼
sleap/gui/commands.py
Outdated
| if len(prev_instances) > len(context.state["labeled_frame"].instances): | ||
| # If more instances in previous frame than current, then use the | ||
| # first unmatched instance. | ||
| copy_instance = prev_instances[ | ||
| len(context.state["labeled_frame"].instances) | ||
| ] | ||
|
|
||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in previous frame, if present | ||
| user_instance = AddInstance.find_last_user_instance(prev_frame) | ||
| if user_instance is not None: | ||
| copy_instance = user_instance | ||
|
|
||
| from_prev_frame = True |
There was a problem hiding this comment.
Out of the three ways am instance is copied from the previous frame, this is the only one that doesn't seem to match the intended functionality. This case tries to find an instance that exists in the previous frame, but not in the current frame and add it to the current frame. We can use that same logic, but just prioritize the user instance. Maybe something like:
| if len(prev_instances) > len(context.state["labeled_frame"].instances): | |
| # If more instances in previous frame than current, then use the | |
| # first unmatched instance. | |
| copy_instance = prev_instances[ | |
| len(context.state["labeled_frame"].instances) | |
| ] | |
| if isinstance(copy_instance, PredictedInstance): | |
| # Set copy instance to last user instance in previous frame, if present | |
| user_instance = AddInstance.find_last_user_instance(prev_frame) | |
| if user_instance is not None: | |
| copy_instance = user_instance | |
| from_prev_frame = True | |
| if len(prev_instances) > len(context.state["labeled_frame"].instances): | |
| # If more instances in previous frame than current, then use the | |
| # first unmatched instance. | |
| prev_user_instances = prev_instances.user_instances | |
| current_user_instances = context.state["labeled_frame"].instances | |
| if len(prev_instances.user_instances) > len(context.state["labeled_frame"].user_instances): | |
| user_instance = prev_user_instances[len(current_user_instances)] | |
| else: | |
| copy_instance = prev_instances[ | |
| len(context.state["labeled_frame"].instances) | |
| ] | |
| from_prev_frame = True |
I liked that you added an AddInstance.find_last_user method, maybe we can edit that function a bit to find_user_instance(index: int = -1, frame: LabeledFrame) where we use a default argument for the index to always get the last instance if the index is not provided?
sleap/gui/commands.py
Outdated
| prev_frame = context.labels.find( | ||
| context.state["video"], prev_idx, return_new=True | ||
| )[0].instances | ||
| )[0] | ||
| prev_instances = prev_frame.instances | ||
| if len(prev_instances) > len(context.state["labeled_frame"].instances): | ||
| # If more instances in previous frame than current, then use the | ||
| # first unmatched instance. | ||
| copy_instance = prev_instances[ | ||
| len(context.state["labeled_frame"].instances) | ||
| ] | ||
|
|
||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in previous frame, if present | ||
| user_instance = AddInstance.find_last_user_instance(prev_frame) | ||
| if user_instance is not None: | ||
| copy_instance = user_instance | ||
|
|
||
| from_prev_frame = True | ||
| elif init_method == "best" and ( | ||
| context.state["labeled_frame"].instances | ||
| ): | ||
| # Otherwise, if there are already instances in current frame, | ||
| # copy the points from the last instance added to frame. | ||
| copy_instance = context.state["labeled_frame"].instances[-1] | ||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in current frame | ||
| user_instance = AddInstance.find_last_user_instance( | ||
| context.state["labeled_frame"] | ||
| ) | ||
| if user_instance is not None: | ||
| copy_instance = user_instance | ||
|
|
||
| elif len(prev_instances): | ||
| # Otherwise use the last instance added to previous frame. | ||
| copy_instance = prev_instances[-1] | ||
|
|
||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in previous frame, if present | ||
| user_instance = AddInstance.find_last_user_instance(prev_frame) | ||
| if user_instance is not None: | ||
| copy_instance = user_instance | ||
|
|
There was a problem hiding this comment.
I agree, the same pattern is used over an over, maybe we can just include that into the new method you created.
tests/gui/test_commands.py
Outdated
| prev_instances.append(user_inst) | ||
|
|
||
| # Confirm there is one user instance in previous frame | ||
| assert len(prev_frame.user_instances) == 1 | ||
|
|
||
| context.newInstance(init_method="prior_frame") | ||
|
|
||
| # Confirm that the newly added user instance is the same as the sole user instance in the previous frame+ | ||
| newly_added_instance = context.state["labeled_frame"].user_instances[0] |
There was a problem hiding this comment.
Maybe a better check would be to insert the user_inst (instead of append) since the original logic that we wanted to change would have just taken the last instance in the prev_instances list in any case. I would also add assertion statements here to ensure that we are in the case that you want to test (and to help me, the reviewer), i.e. for the first case:
assert len(prev_instances) > len(context.state["labeled_frame"].instances):
tests/gui/test_commands.py
Outdated
| assert newly_added_instance.track == user_inst.track | ||
|
|
||
|
|
||
| def test_CopyPriorFramePreviousUser2(centered_pair_predictions: Labels): |
There was a problem hiding this comment.
Let's use a better name that describes what the 2 means, or you could bundle it into the previous test if you want and section with assert statements, i.e. for the second case we would make some changes to our labels to enter the second case and:
assert len(prev_instances) <= len(context.state["labeled_frame"].instances) # not the first case
assert context.state["labeled_frame"].instances # but yes to the second case
tests/gui/test_commands.py
Outdated
| prev_instances.insert(0, user_inst) | ||
|
|
||
| # Confirm addition of user instance in previous frame | ||
| assert len(prev_frame.user_instances) == 1 | ||
|
|
||
| context.newInstance(init_method="best") | ||
|
|
||
| # Confirm that user instance in current frame is the same as the user instance in previous frame | ||
| current_user_instance = context.state["labeled_frame"].user_instances[0] |
There was a problem hiding this comment.
Great! yes the insert! The only comment here would be to add the assert statement for which case we are in:
assert len(prev_instances) <= len(context.state["labeled_frame"].instances) # no to the first case
assert context.state["labeled_frame"].instances # but yes to the second case
tests/gui/test_commands.py
Outdated
| """Test that we copy user instance in current frame when the current frame has >= amount of instances | ||
| as the previous frame.""" |
There was a problem hiding this comment.
Nit: this is a bit too long of a summary doc string (see google style guide), but I am super pleased that you added some docstrings in the first place!
| # Confirm that current frame has more instances than previous frame | ||
| assert len(context.state["labeled_frame"].instances) > len(prev_frame.instances) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/commands.py (2 hunks)
- tests/gui/test_commands.py (3 hunks)
Additional comments: 4
tests/gui/test_commands.py (2)
- 21-21: The import of the
Pointclass fromsleap.instanceis correctly added to support the new test cases that involve creating instances with points.- 619-782: The new test functions
test_CopyPriorFrame_MoreInstancesInPreviousFrame,test_CopyPriorFrame_EquivalentInstancesInCurrentAndPreviousFrame, andtest_CopyPriorFrame_GetFromCurrentFrameare well-structured and directly address the PR objectives by testing the prioritization of user instances over predicted instances in various scenarios. Each test function correctly sets up the context and labeled frames to simulate the conditions described in their docstrings, ensuring comprehensive coverage of the new logic introduced in the PR.
- The tests effectively manipulate the
CommandContextand labeled frames to create scenarios with different numbers of user and predicted instances across frames.- The use of assertions to verify the conditions before and after the operation (e.g., the number of user instances and their properties) is appropriate and ensures the correctness of the new functionality.
- The clear and descriptive docstrings provide valuable context for understanding the purpose and expected behavior of each test, aligning with best practices for test documentation.
Overall, these tests are well-implemented and effectively validate the new functionality introduced in the PR.
sleap/gui/commands.py (2)
- 3098-3141: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3022-3138]
The integration of
find_last_user_instanceandreplace_with_user_instance_if_neededwithinfind_instance_to_copy_frommethod correctly implements the logic to prioritize user instances. However, there's a potential issue with the handling offrom_predictedandfrom_prev_frameflags, which could lead to incorrect assumptions about the source of the copied instance. Ensure these flags are accurately set based on the actual source of the instance being copied.Consider adding unit tests to verify the behavior of
find_instance_to_copy_fromwhen dealing with different scenarios of user and predicted instances in previous frames.
- 3098-3141: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3022-3138]
Overall, the changes made in the
sleap/gui/commands.pyfile align with the PR objectives to prioritize user instances when copying from a prior frame. The addition of new methods and modifications to existing logic are well-thought-out. However, ensure thorough testing, especially around edge cases where the previous frame might not contain any user instances or when multiple instances (user and predicted) are present.While the changes are approved, consider the suggestions made for improving documentation and handling edge cases.
sleap/gui/commands.py
Outdated
| @staticmethod | ||
| def find_last_user_instance( | ||
| frame_to_copy_from: LabeledFrame, | ||
| index: int = -1, | ||
| ) -> Optional[Instance]: | ||
| """Find last user instance to copy from | ||
|
|
||
| Args: | ||
| frame_to_copy_from: The last labeled frame from which we obtain the last user instance. | ||
|
|
||
| Returns: | ||
| The last user instance in the previous frame (if present), otherwise null | ||
| """ | ||
|
|
||
| user_instances = frame_to_copy_from.user_instances | ||
| if len(user_instances) > 0: | ||
| return user_instances[index] | ||
|
|
||
| @staticmethod | ||
| def replace_with_user_instance_if_needed( | ||
| copy_instance: Optional[Union[Instance, PredictedInstance]], | ||
| frame_to_copy_from: LabeledFrame, | ||
| ): | ||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in frame to copy from, if present | ||
| user_instance = AddInstance.find_last_user_instance(frame_to_copy_from) | ||
| if user_instance is not None: | ||
| return user_instance | ||
|
|
||
| return copy_instance |
There was a problem hiding this comment.
The method find_last_user_instance correctly retrieves the last user instance from a given frame, which aligns with the PR's objective to prioritize user instances. However, consider handling the case where index might be out of bounds for user_instances.
if len(user_instances) > 0 and abs(index) <= len(user_instances):
return user_instances[index]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.
| @staticmethod | |
| def find_last_user_instance( | |
| frame_to_copy_from: LabeledFrame, | |
| index: int = -1, | |
| ) -> Optional[Instance]: | |
| """Find last user instance to copy from | |
| Args: | |
| frame_to_copy_from: The last labeled frame from which we obtain the last user instance. | |
| Returns: | |
| The last user instance in the previous frame (if present), otherwise null | |
| """ | |
| user_instances = frame_to_copy_from.user_instances | |
| if len(user_instances) > 0: | |
| return user_instances[index] | |
| @staticmethod | |
| def replace_with_user_instance_if_needed( | |
| copy_instance: Optional[Union[Instance, PredictedInstance]], | |
| frame_to_copy_from: LabeledFrame, | |
| ): | |
| if isinstance(copy_instance, PredictedInstance): | |
| # Set copy instance to last user instance in frame to copy from, if present | |
| user_instance = AddInstance.find_last_user_instance(frame_to_copy_from) | |
| if user_instance is not None: | |
| return user_instance | |
| return copy_instance | |
| if len(user_instances) > 0 and abs(index) <= len(user_instances): | |
| return user_instances[index] |
sleap/gui/commands.py
Outdated
| def replace_with_user_instance_if_needed( | ||
| copy_instance: Optional[Union[Instance, PredictedInstance]], | ||
| frame_to_copy_from: LabeledFrame, | ||
| ): | ||
| if isinstance(copy_instance, PredictedInstance): | ||
| # Set copy instance to last user instance in frame to copy from, if present | ||
| user_instance = AddInstance.find_last_user_instance(frame_to_copy_from) | ||
| if user_instance is not None: | ||
| return user_instance | ||
|
|
||
| return copy_instance |
There was a problem hiding this comment.
The method replace_with_user_instance_if_needed effectively replaces a predicted instance with a user instance if available. This method supports the PR's goal but lacks documentation on its parameters and return type, which could improve code readability and maintainability.
+ """
+ Replaces a predicted instance with a user instance if available.
+
+ Args:
+ copy_instance: The instance to potentially replace.
+ frame_to_copy_from: The frame from which to find a user instance.
+
+ Returns:
+ The user instance if found; otherwise, returns the original instance.
+ """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.
| def replace_with_user_instance_if_needed( | |
| copy_instance: Optional[Union[Instance, PredictedInstance]], | |
| frame_to_copy_from: LabeledFrame, | |
| ): | |
| if isinstance(copy_instance, PredictedInstance): | |
| # Set copy instance to last user instance in frame to copy from, if present | |
| user_instance = AddInstance.find_last_user_instance(frame_to_copy_from) | |
| if user_instance is not None: | |
| return user_instance | |
| return copy_instance | |
| def replace_with_user_instance_if_needed( | |
| copy_instance: Optional[Union[Instance, PredictedInstance]], | |
| frame_to_copy_from: LabeledFrame, | |
| ): | |
| """ | |
| Replaces a predicted instance with a user instance if available. | |
| Args: | |
| copy_instance: The instance to potentially replace. | |
| frame_to_copy_from: The frame from which to find a user instance. | |
| Returns: | |
| The user instance if found; otherwise, returns the original instance. | |
| """ | |
| if isinstance(copy_instance, PredictedInstance): | |
| # Set copy instance to last user instance in frame to copy from, if present | |
| user_instance = AddInstance.find_last_user_instance(frame_to_copy_from) | |
| if user_instance is not None: | |
| return user_instance | |
| return copy_instance |
Description
Prioritised copy prior frame to take user instance when available.
Types of changes
Does this address any currently open issues?
[list open issues here]
#1065
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Tests