-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add menu item for deleting instances beyond frame limit #1797
base: develop
Are you sure you want to change the base?
Add menu item for deleting instances beyond frame limit #1797
Conversation
WalkthroughA new feature has been introduced to the SLEAP GUI application, allowing users to delete predicted instances beyond a specified frame limit. This involves adding a new menu item, creating a command to handle the deletion, and implementing tests to ensure functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GUI
participant Commands
participant DataHandler
User->>GUI: Clicks "Delete Predictions beyond Frame Limit..."
GUI->>Commands: Trigger deleteFrameLimitPredictions()
Commands->>DataHandler: Identify and delete instances beyond frame limit
DataHandler->>Commands: Confirm deletion
Commands->>GUI: Update display
GUI->>User: Show updated predictions
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (6)
sleap/gui/app.py (1)
Line range hint
261-261
: Remove the unused variablefilename
to clean up the code.- filename = event.mimeData().data(mime_format).data().decode()
sleap/gui/commands.py (5)
Line range hint
193-193
: Undefined reference toMainWindow
.The reference to
MainWindow
in theexecute
method ofNewProject
class is undefined within this file. Ensure thatMainWindow
is correctly imported or defined to avoid runtime errors.
Line range hint
817-817
: Unused variablefile_dir
.The variable
file_dir
in theask
method ofImportNWB
is assigned but never used. Consider removing it if it's not needed.
Line range hint
1691-1691
: Avoid using bareexcept
.Using a bare
except
is not recommended as it can catch unexpected exceptions and hide programming errors. Specify the exception types that you expect to handle.Also applies to: 1712-1712
Line range hint
2418-2418
: Remove unnecessary f-string.The f-string in this line does not contain any placeholders, making the use of an f-string unnecessary. Consider using a regular string instead.
Also applies to: 2787-2787
Line range hint
3149-3149
: Avoid using bareexcept
.Using a bare
except
is not recommended as it can catch unexpected exceptions and hide programming errors. Specify the exception types that you expect to handle.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (2 hunks)
Additional context used
Ruff
sleap/gui/app.py
261-261: Local variable
filename
is assigned to but never used (F841)sleap/gui/commands.py
193-193: Undefined name
MainWindow
(F821)
817-817: Local variable
file_dir
is assigned to but never used (F841)
1691-1691: Do not use bare
except
(E722)
1712-1712: Do not use bare
except
(E722)
2418-2418: f-string without any placeholders (F541)
2787-2787: f-string without any placeholders (F541)
3149-3149: Do not use bare
except
(E722)
Additional comments not posted (1)
sleap/gui/app.py (1)
793-795
: The addition of the new menu item "Delete Predictions beyond Frame Limit new item..." is correctly implemented and linked to the appropriate command.
def deleteFrameLimitPredictions(self): | ||
"""Gui for deleting instances beyond some frame number.""" | ||
self.execute(DeleteFrameLimitPredictions) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add method documentation for deleteFrameLimitPredictions
.
Consider adding a detailed docstring for the deleteFrameLimitPredictions
method to explain its parameters, return type, and any exceptions it might raise. This will enhance code maintainability and readability.
sleap/gui/commands.py
Outdated
class DeleteFrameLimitPredictions(InstanceDeleteCommand): | ||
@staticmethod | ||
def get_frame_instance_list(context: CommandContext, params: Dict): | ||
predicted_instances = [] | ||
# Select the instances to be deleted | ||
for lf in context.labels.find(context.state["video"]): | ||
if lf.frame_idx >= params["frame_idx_threshold"]: | ||
predicted_instances.extend( | ||
[(lf, inst) for inst in lf.predicted_instances] | ||
) | ||
return predicted_instances | ||
|
||
@classmethod | ||
def ask(cls, context: CommandContext, params: Dict) -> bool: | ||
current_video = context.state["video"] | ||
frame_idx_thresh, okay = QtWidgets.QInputDialog.getInt( | ||
context.app, | ||
"Delete Instance beyond Frame Number...", | ||
"Frame number after which instances to be deleted:", | ||
1, | ||
1, | ||
len(current_video), | ||
) | ||
if okay: | ||
params["frame_idx_threshold"] = frame_idx_thresh | ||
return super().ask(context, params) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper validation and error handling in DeleteFrameLimitPredictions
.
- if okay:
+ if okay and frame_idx_thresh > 0:
It's crucial to validate the frame_idx_threshold
to ensure it's a positive integer. This prevents logical errors where a negative frame index could potentially delete all instances.
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.
class DeleteFrameLimitPredictions(InstanceDeleteCommand): | |
@staticmethod | |
def get_frame_instance_list(context: CommandContext, params: Dict): | |
predicted_instances = [] | |
# Select the instances to be deleted | |
for lf in context.labels.find(context.state["video"]): | |
if lf.frame_idx >= params["frame_idx_threshold"]: | |
predicted_instances.extend( | |
[(lf, inst) for inst in lf.predicted_instances] | |
) | |
return predicted_instances | |
@classmethod | |
def ask(cls, context: CommandContext, params: Dict) -> bool: | |
current_video = context.state["video"] | |
frame_idx_thresh, okay = QtWidgets.QInputDialog.getInt( | |
context.app, | |
"Delete Instance beyond Frame Number...", | |
"Frame number after which instances to be deleted:", | |
1, | |
1, | |
len(current_video), | |
) | |
if okay: | |
params["frame_idx_threshold"] = frame_idx_thresh | |
return super().ask(context, params) | |
class DeleteFrameLimitPredictions(InstanceDeleteCommand): | |
@staticmethod | |
def get_frame_instance_list(context: CommandContext, params: Dict): | |
predicted_instances = [] | |
# Select the instances to be deleted | |
for lf in context.labels.find(context.state["video"]): | |
if lf.frame_idx >= params["frame_idx_threshold"]: | |
predicted_instances.extend( | |
[(lf, inst) for inst in lf.predicted_instances] | |
) | |
return predicted_instances | |
@classmethod | |
def ask(cls, context: CommandContext, params: Dict) -> bool: | |
current_video = context.state["video"] | |
frame_idx_thresh, okay = QtWidgets.QInputDialog.getInt( | |
context.app, | |
"Delete Instance beyond Frame Number...", | |
"Frame number after which instances to be deleted:", | |
1, | |
1, | |
len(current_video), | |
) | |
if okay and frame_idx_thresh > 0: | |
params["frame_idx_threshold"] = frame_idx_thresh | |
return super().ask(context, params) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1797 +/- ##
===========================================
+ Coverage 73.30% 74.16% +0.85%
===========================================
Files 134 135 +1
Lines 24087 24575 +488
===========================================
+ Hits 17658 18226 +568
+ Misses 6429 6349 -80 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (22)
tests/gui/test_commands.py (6)
Line range hint
76-76
: Remove duplicate items from sets to ensure data integrity.- "tests/data/dlc_multiple_datasets/video1/img000.jpg", - "tests/data/dlc_multiple_datasets/video1/img000.jpg", + "tests/data/dlc_multiple_datasets/video1/img000.jpg",- {0, 0, 1} + {0, 1}Also applies to: 79-79
Line range hint
134-137
: Simplify the assignment ofcsv
using a ternary operator.- if out_suffix == "csv": - csv = True - else: - csv = False + csv = True if out_suffix == "csv" else False
Line range hint
225-225
: Replace equality comparisons toTrue
with direct truth checks.- assert okay == True + assert okayAlso applies to: 234-234, 242-242, 251-251, 263-263, 272-272, 279-279, 297-297
Line range hint
358-358
: Remove the unused variablelast_lf_frame
to clean up the code.- last_lf_frame = get_last_lf_in_video(labels, videos[0])
Line range hint
366-366
: Use direct truth checks instead of comparing toTrue
.- assert video.backend.grayscale == True + assert video.backend.grayscale
Line range hint
526-526
: Remove the extraneousf
prefix from the string since there are no placeholders.- default_name += f".{adaptor.default_ext}" + default_name += ".{adaptor.default_ext}"sleap/gui/app.py (6)
Line range hint
123-123
: Usesuper()
instead ofsuper(__class__, self)
.- super(MainWindow, self).__init__(*args, **kwargs) + super().__init__(*args, **kwargs)
Line range hint
194-194
: Usesuper()
instead ofsuper(__class__, self)
.- super(MainWindow, self).setWindowTitle(f"{value} - SLEAP v{sleap.version.__version__}") + super().setWindowTitle(f"{value} - SLEAP v{sleap.version.__version__}")
Line range hint
209-210
: Combine nestedif
statements usingand
.- if e.type() == QEvent.StatusTip: - if e.tip() == "": + if e.type() == QEvent.StatusTip and e.tip() == "":
Line range hint
261-261
: Local variablefilename
is assigned to but never used.- filename = event.mimeData().data(mime_format).data().decode()
Line range hint
1087-1087
: Remove extraneous parentheses.- if (n_instances > 0) and not self.state["show instances"]: + if n_instances > 0 and not self.state["show instances"]:
Line range hint
1148-1151
: Use a more concise expression withany()
.- def _has_topic(topic_list): - if UpdateTopic.all in what: - return True - for topic in topic_list: - if topic in what: - return True - return False + def _has_topic(topic_list): + return UpdateTopic.all in what or any(topic in what for topic in topic_list)sleap/gui/commands.py (10)
Line range hint
193-193
: Undefined reference toMainWindow
.Please ensure that
MainWindow
is defined or imported in this file, as it is referenced in theCommandContext
class.
Line range hint
653-653
: Simplify dictionary access.- params.get("filename", None) + params.get("filename")
Line range hint
756-756
: Simplify dictionary access.- video_path = params["video_path"] if "video_path" in params else None + video_path = params.get("video_path", None)
Line range hint
817-817
: Remove unused variable.- file_dir = os.path.dirname(filename)
The variable
file_dir
is assigned but never used. Consider removing it to clean up the code.
Line range hint
1691-1691
: Avoid using bareexcept
.Using bare
except
can catch unexpected exceptions and hide programming errors. Specify the exception type or useexcept Exception
to catch all standard errors.Also applies to: 1712-1712
Line range hint
1972-1972
: Improve exception chaining.- except Exception as e: + except Exception as e: + raise RuntimeError("Failed to save project.") from eUse
raise ... from
to provide context for the exception and help with debugging.
Line range hint
2101-2101
: Simplify dictionary key check.- if node not in instance.nodes.keys(): + if node not in instance.nodes:You can directly check for the existence of a key in a dictionary without using
.keys()
.Also applies to: 2116-2116
Line range hint
2418-2418
: Remove unnecessary f-string prefix.- f"This video type {type(video.backend)} for video at index {idx} " + "This video type {type(video.backend)} for video at index {idx} "The f-string is used without placeholders, so the prefix can be removed.
Also applies to: 2789-2789
Line range hint
2887-2887
: Simplify dictionary access.- params.get("copy_instance", None) + params.get("copy_instance") - params.get("location", None) + params.get("location")Also applies to: 2889-2889
Line range hint
3151-3151
: Avoid using bareexcept
.Using bare
except
can catch unexpected exceptions and hide programming errors. Specify the exception type or useexcept Exception
to catch all standard errors.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (2 hunks)
- tests/gui/test_commands.py (2 hunks)
Additional context used
Ruff
tests/gui/test_commands.py
72-72: Ambiguous variable name:
l
(E741)
76-76: Sets should not contain duplicate item
"tests/data/dlc_multiple_datasets/video1/img000.jpg"
(B033)Remove duplicate item
79-79: Ambiguous variable name:
l
(E741)
79-79: Sets should not contain duplicate item
0
(B033)Remove duplicate item
134-137: Use ternary operator
csv = True if out_suffix == "csv" else False
instead ofif
-else
-block (SIM108)Replace
if
-else
-block withcsv = True if out_suffix == "csv" else False
200-200: Loop control variable
video
not used within loop body (B007)Rename unused
video
to_video
225-225: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
234-234: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
242-242: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
251-251: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
263-263: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
272-272: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
279-279: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
297-297: Avoid equality comparisons to
True
; useif okay:
for truth checks (E712)Replace with
okay
303-303: Loop control variable
video
not used within loop body (B007)Rename unused
video
to_video
358-358: Local variable
last_lf_frame
is assigned to but never used (F841)Remove assignment to unused variable
last_lf_frame
366-366: Avoid equality comparisons to
True
; useif video.backend.grayscale:
for truth checks (E712)Replace with
video.backend.grayscale
526-526: f-string without any placeholders (F541)
Remove extraneous
f
prefixsleap/gui/app.py
123-123: Use
super()
instead ofsuper(__class__, self)
(UP008)Remove
__super__
parameters
194-194: Use
super()
instead ofsuper(__class__, self)
(UP008)Remove
__super__
parameters
209-210: Use a single
if
statement instead of nestedif
statements (SIM102)Combine
if
statements usingand
261-261: Local variable
filename
is assigned to but never used (F841)Remove assignment to unused variable
filename
1087-1087: Avoid extraneous parentheses (UP034)
Remove extraneous parentheses
1148-1151: Use
return any(topic in what for topic in topic_list)
instead offor
loop (SIM110)Replace with
return any(topic in what for topic in topic_list)
sleap/gui/commands.py
193-193: Undefined name
MainWindow
(F821)
653-653: Use
params.get("filename")
instead ofparams.get("filename", None)
(SIM910)Replace
params.get("filename", None)
withparams.get("filename")
756-756: Use
params.get("video_path", None)
instead of anif
block (SIM401)Replace with
params.get("video_path", None)
817-817: Local variable
file_dir
is assigned to but never used (F841)Remove assignment to unused variable
file_dir
1691-1691: Do not use bare
except
(E722)
1712-1712: Do not use bare
except
(E722)
1972-1972: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling (B904)
2101-2101: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2116-2116: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2418-2418: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2789-2789: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2887-2887: Use
params.get("copy_instance")
instead ofparams.get("copy_instance", None)
(SIM910)Replace
params.get("copy_instance", None)
withparams.get("copy_instance")
2889-2889: Use
params.get("location")
instead ofparams.get("location", None)
(SIM910)Replace
params.get("location", None)
withparams.get("location")
3151-3151: Do not use bare
except
(E722)
Additional comments not posted (3)
tests/gui/test_commands.py (2)
19-19
: The import ofDeleteFrameLimitPredictions
is correctly placed in the import section. Ensure that all necessary methods from this class are covered by unit tests.
851-871
: The new test functiontest_DeleteFrameLimitPredictions
is well-structured and tests the functionality as described in the PR. Ensure that theexpected_instances
value is derived from a reliable source or calculation to maintain test accuracy.sleap/gui/app.py (1)
791-796
: Ensure the new menu item "Delete Predictions beyond Frame Limit..." is correctly integrated and accessible.This addition aligns with the PR's objective to enhance usability by allowing users to delete predictions beyond a specified frame limit. Please ensure that this new menu item is tested thoroughly to confirm its functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (10)
sleap/gui/commands.py (10)
Line range hint
193-193
: Undefined reference toMainWindow
.The reference to
MainWindow
in the methodexecute
of theAppCommand
class is undefined within this file. Ensure thatMainWindow
is properly imported or defined, or use a different existing reference.- MainWindow + [Correct reference or import statement]
Line range hint
653-653
: Simplify dictionary access.Use
params.get("filename")
instead ofparams.get("filename", None)
for simplicity and readability.- params.get("filename", None) + params.get("filename")
Line range hint
756-756
: Simplify dictionary access.Use
params.get("video_path", None)
instead of anif
block for simplicity and readability.- video_path = params["video_path"] if "video_path" in params else None + video_path = params.get("video_path", None)
Line range hint
817-817
: Remove unused variable.The local variable
file_dir
is assigned but never used. Consider removing it to clean up the code.- file_dir = os.path.dirname(filename)
Line range hint
1691-1691
: Avoid using bareexcept
.Using a bare
except
can catch unexpected exceptions and hide programming errors. Specify the exception type to improve error handling.- except: + except Exception as e:Also applies to: 1712-1712
Line range hint
1972-1972
: Improve exception chaining.Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling.- raise TypeError("Importing videos with different extensions is not supported.") + raise TypeError("Importing videos with different extensions is not supported.") from None
Line range hint
2101-2101
: Usekey in dict
instead ofkey in dict.keys()
.For checking if a key is in a dictionary, it's more idiomatic and slightly faster to use
key in dict
instead ofkey in dict.keys()
.- if node not in instance.nodes.keys(): + if node not in instance.nodes:Also applies to: 2116-2116
Line range hint
2418-2418
: Remove extraneousf
prefix from strings.The
f
prefix is used for formatted strings in Python, but it's unnecessary when the string does not contain any placeholders.- f"This video type {type(video.backend)} for video at index {idx} does not support grayscale yet." + "This video type does not support grayscale yet."Also applies to: 2789-2789
Line range hint
2887-2887
: Simplify dictionary access.Use
params.get("copy_instance")
andparams.get("location")
instead ofparams.get("copy_instance", None)
andparams.get("location", None)
for simplicity and readability.- params.get("copy_instance", None) + params.get("copy_instance") - params.get("location", None) + params.get("location")Also applies to: 2889-2889
Line range hint
3151-3151
: Avoid using bareexcept
.Using a bare
except
can catch unexpected exceptions and hide programming errors. Specify the exception type to improve error handling.- except: + except Exception as e:
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (2 hunks)
Additional context used
Ruff
sleap/gui/commands.py
193-193: Undefined name
MainWindow
(F821)
653-653: Use
params.get("filename")
instead ofparams.get("filename", None)
(SIM910)Replace
params.get("filename", None)
withparams.get("filename")
756-756: Use
params.get("video_path", None)
instead of anif
block (SIM401)Replace with
params.get("video_path", None)
817-817: Local variable
file_dir
is assigned to but never used (F841)Remove assignment to unused variable
file_dir
1691-1691: Do not use bare
except
(E722)
1712-1712: Do not use bare
except
(E722)
1972-1972: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling (B904)
2101-2101: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2116-2116: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2418-2418: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2789-2789: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2887-2887: Use
params.get("copy_instance")
instead ofparams.get("copy_instance", None)
(SIM910)Replace
params.get("copy_instance", None)
withparams.get("copy_instance")
2889-2889: Use
params.get("location")
instead ofparams.get("location", None)
(SIM910)Replace
params.get("location", None)
withparams.get("location")
3151-3151: Do not use bare
except
(E722)
Additional comments not posted (2)
sleap/gui/commands.py (2)
497-500
: Add method documentation fordeleteFrameLimitPredictions
.Consider adding a detailed docstring for the
deleteFrameLimitPredictions
method to explain its parameters, return type, and any exceptions it might raise. This will enhance code maintainability and readability.
2476-2487
: Add a comment that this is used in theInstanceDeleteCommand.ask
method.
sleap/gui/commands.py
Outdated
"""Called from the parent `InstanceDeleteCommand.ask` class. Returns a list of | ||
instances to be deleted.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a single line summary docstring followed by Args and Returns if needed.
…ps://github.com/talmolab/sleap into shrivaths/delete-predictions-beyond-frame-limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (10)
sleap/gui/commands.py (10)
Line range hint
193-193
: Undefined reference toMainWindow
.- """Entry point for running command. + """Entry point for running command in `MainWindow`.Ensure that
MainWindow
is correctly imported or defined within the scope where it's referenced.
Line range hint
653-653
: Simplify theget
method for dictionary access.- filename = params.get("filename", None) # If called with just a Labels object + filename = params.get("filename") # If called with just a Labels object
Line range hint
756-756
: Use theget
method with a default value directly.- video_path = params["video_path"] if "video_path" in params else None + video_path = params.get("video_path", None)
Line range hint
817-817
: Remove assignment to unused variablefile_dir
.- file_dir = os.path.dirname(filename)
Line range hint
1691-1691
: Avoid using bareexcept
statements. Specify exception types to handle specific exceptions appropriately.- except: + except Exception as e:Also applies to: 1712-1712
Line range hint
1972-1972
: When re-raising exceptions, useraise ... from err
to provide context.- raise e + raise Exception("Error during save operation.") from e
Line range hint
2101-2101
: Usekey in dict
instead ofkey in dict.keys()
.- if node not in instance.nodes.keys(): + if node not in instance.nodes:Also applies to: 2116-2116
Line range hint
2418-2418
: Remove extraneousf
prefix from strings that do not contain placeholders.- f"This video type {type(video.backend)} for video at index {idx} " + "This video type {type(video.backend)} for video at index {idx} "Also applies to: 2792-2792
Line range hint
2890-2890
: Simplify theget
method for dictionary access.- copy_instance = params.get("copy_instance", None) + copy_instance = params.get("copy_instance") - location = params.get("location", None) + location = params.get("location")Also applies to: 2892-2892
Line range hint
3154-3154
: Avoid using bareexcept
statements. Specify exception types to handle specific exceptions appropriately.- except: + except Exception as e:
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (2 hunks)
Additional context used
Ruff
sleap/gui/commands.py
193-193: Undefined name
MainWindow
(F821)
653-653: Use
params.get("filename")
instead ofparams.get("filename", None)
(SIM910)Replace
params.get("filename", None)
withparams.get("filename")
756-756: Use
params.get("video_path", None)
instead of anif
block (SIM401)Replace with
params.get("video_path", None)
817-817: Local variable
file_dir
is assigned to but never used (F841)Remove assignment to unused variable
file_dir
1691-1691: Do not use bare
except
(E722)
1712-1712: Do not use bare
except
(E722)
1972-1972: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling (B904)
2101-2101: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2116-2116: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2418-2418: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2792-2792: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2890-2890: Use
params.get("copy_instance")
instead ofparams.get("copy_instance", None)
(SIM910)Replace
params.get("copy_instance", None)
withparams.get("copy_instance")
2892-2892: Use
params.get("location")
instead ofparams.get("location", None)
(SIM910)Replace
params.get("location", None)
withparams.get("location")
3154-3154: Do not use bare
except
(E722)
Additional comments not posted (2)
sleap/gui/commands.py (2)
497-500
: Consider adding a detailed docstring for thedeleteFrameLimitPredictions
method to explain its parameters, return type, and any exceptions it might raise. This will enhance code maintainability and readability.
2476-2477
: Add a comment that this is used in theInstanceDeleteCommand.ask
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (10)
sleap/gui/commands.py (10)
Line range hint
193-193
: Specify correct type hint forMainWindow
.The type hint for
MainWindow
is missing, causing an undefined name issue. Please importMainWindow
or define it if it's a custom class.+ from sleap.gui.mainwindow import MainWindow
Line range hint
653-653
: Simplify dictionary access.Directly use
params.get("filename")
instead ofparams.get("filename", None)
asNone
is the default return value for theget
method.- filename = params.get("filename", None) # If called with just a Labels object + filename = params.get("filename") # If called with just a Labels object
Line range hint
756-756
: Simplify dictionary access.Replace the
if
block withparams.get("video_path", None)
for cleaner and more efficient code.- video_path = params["video_path"] if "video_path" in params else None + video_path = params.get("video_path", None)
Line range hint
817-817
: Remove unused variable.The variable
file_dir
is assigned but never used. Consider removing it to clean up the code.- file_dir = os.path.dirname(filename)
Line range hint
1691-1691
: Avoid using bareexcept
statements.Using bare
except
statements can catch unexpected exceptions and hide programming errors. Specify the exception type or useexcept Exception
to catch all standard exceptions.- except: + except Exception:Also applies to: 1712-1712
Line range hint
1972-1972
: Improve exception chaining for clarity.When re-raising exceptions, use the
from
keyword to clarify whether the new exception was directly caused by the previous one.- except Exception as e: + except Exception as e: + raise RuntimeError("Failed to save") from e
Line range hint
2101-2101
: Simplify dictionary key check.Use
key in dict
instead ofkey in dict.keys()
for more concise and pythonic code.- if node in instance.nodes.keys(): + if node in instance.nodes:Also applies to: 2116-2116
Line range hint
2420-2420
: Remove unnecessary f-string prefix.The f-string syntax is used without placeholders, which is unnecessary. Remove the
f
prefix.- f"This video type {type(video.backend)} for video at index {idx} " + "This video type {type(video.backend)} for video at index {idx} "Also applies to: 2796-2796
Line range hint
2894-2894
: Simplify dictionary access.Use
params.get("key")
directly as it is cleaner and the default return isNone
.- copy_instance = params.get("copy_instance", None) + copy_instance = params.get("copy_instance") - location = params.get("location", None) + location = params.get("location")Also applies to: 2896-2896
Line range hint
3158-3158
: Avoid using bareexcept
statements.Replace bare
except
withexcept Exception
to avoid catching unexpected exceptions.- except: + except Exception:
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/commands.py (2 hunks)
Additional context used
Ruff
sleap/gui/commands.py
193-193: Undefined name
MainWindow
(F821)
653-653: Use
params.get("filename")
instead ofparams.get("filename", None)
(SIM910)Replace
params.get("filename", None)
withparams.get("filename")
756-756: Use
params.get("video_path", None)
instead of anif
block (SIM401)Replace with
params.get("video_path", None)
817-817: Local variable
file_dir
is assigned to but never used (F841)Remove assignment to unused variable
file_dir
1691-1691: Do not use bare
except
(E722)
1712-1712: Do not use bare
except
(E722)
1972-1972: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling (B904)
2101-2101: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2116-2116: Use
key in dict
instead ofkey in dict.keys()
(SIM118)Remove
.keys()
2420-2420: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2796-2796: f-string without any placeholders (F541)
Remove extraneous
f
prefix
2894-2894: Use
params.get("copy_instance")
instead ofparams.get("copy_instance", None)
(SIM910)Replace
params.get("copy_instance", None)
withparams.get("copy_instance")
2896-2896: Use
params.get("location")
instead ofparams.get("location", None)
(SIM910)Replace
params.get("location", None)
withparams.get("location")
3158-3158: Do not use bare
except
(E722)
Additional comments not posted (2)
sleap/gui/commands.py (2)
497-500
: Add method documentation fordeleteFrameLimitPredictions
.Consider adding a detailed docstring for the
deleteFrameLimitPredictions
method to explain its parameters, return type, and any exceptions it might raise. This will enhance code maintainability and readability.
2478-2479
: Add a comment that this is used in theInstanceDeleteCommand.ask
method.This comment helps clarify the relationship between the
DeleteFrameLimitPredictions
class and its superclass, making the code easier to understand.
Description
Currently there is no menu option to delete predictions beyond a certain frame number. This would add some ease of use when trying to delete predictions and then re-labeling to train the model again to get different/better predictions.
This came in as a suggestion in #1762
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