Skip to content
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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

shrivaths16
Copy link
Contributor

@shrivaths16 shrivaths16 commented Jun 5, 2024

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

  • Bugfix
  • New feature
  • Refactor / Code style update (no logical changes)
  • Build / CI changes
  • Documentation Update
  • Other (explain)

Does this address any currently open issues?

[list open issues here]

Outside contributors checklist

  • Review the guidelines for contributing to this repository
  • Read and sign the CLA and add yourself to the authors list
  • Make sure you are making a pull request against the develop branch (not main). Also you should start your branch off develop
  • Add tests that prove your fix is effective or that your feature works
  • Add necessary documentation (if appropriate)

Thank you for contributing to SLEAP!

❤️

Summary by CodeRabbit

  • New Features

    • Introduced a new menu item "Delete Predictions beyond Frame Limit..." in the label menu.
    • Added functionality to delete predictions beyond a specified frame limit.
  • Tests

    • Added a test function to ensure correct deletion of instances beyond a specified frame limit.

Copy link

coderabbitai bot commented Jun 5, 2024

Walkthrough

A 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

Files Change Summary
sleap/gui/app.py Added a new menu item "Delete Predictions beyond Frame Limit..." and linked it to a new command.
sleap/gui/commands.py Added deleteFrameLimitPredictions method and DeleteFrameLimitPredictions class for frame-based deletion logic.
tests/gui/test_commands.py Introduced test test_DeleteFrameLimitPredictions to verify the new frame limit deletion functionality.

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
Loading

Poem

In SLEAP's GUI, a feature quite grand,
Deleting predictions with a swift hand.
Beyond frame limits, we clear the slate,
Bringing order, rest assured, it’s first-rate!
With tests to guide, and commands so neat,
Our label makers have tools that can't be beat!
🌟✨🐰


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 variable filename 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 to MainWindow.

The reference to MainWindow in the execute method of NewProject class is undefined within this file. Ensure that MainWindow is correctly imported or defined to avoid runtime errors.


Line range hint 817-817: Unused variable file_dir.

The variable file_dir in the ask method of ImportNWB is assigned but never used. Consider removing it if it's not needed.


Line range hint 1691-1691: Avoid using bare except.

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 bare except.

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

Commits

Files that changed from the base of the PR and between 834c68a and 170048a.

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.

Comment on lines +497 to +500
def deleteFrameLimitPredictions(self):
"""Gui for deleting instances beyond some frame number."""
self.execute(DeleteFrameLimitPredictions)

Copy link

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.

Comment on lines 2475 to 2501
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)

Copy link

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.

Suggested change
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)

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 74.16%. Comparing base (7ed1229) to head (8a7839a).
Report is 16 commits behind head on develop.

Files Patch % Lines
sleap/gui/commands.py 64.70% 6 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a 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 of csv 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 to True with direct truth checks.

- assert okay == True
+ assert okay

Also applies to: 234-234, 242-242, 251-251, 263-263, 272-272, 279-279, 297-297


Line range hint 358-358: Remove the unused variable last_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 to True.

- assert video.backend.grayscale == True
+ assert video.backend.grayscale

Line range hint 526-526: Remove the extraneous f 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: Use super() instead of super(__class__, self).

- super(MainWindow, self).__init__(*args, **kwargs)
+ super().__init__(*args, **kwargs)

Line range hint 194-194: Use super() instead of super(__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 nested if statements using and.

- if e.type() == QEvent.StatusTip:
-     if e.tip() == "":
+ if e.type() == QEvent.StatusTip and e.tip() == "":

Line range hint 261-261: Local variable filename 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 with any().

- 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 to MainWindow.

Please ensure that MainWindow is defined or imported in this file, as it is referenced in the CommandContext 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 bare except.

Using bare except can catch unexpected exceptions and hide programming errors. Specify the exception type or use except 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 e

Use 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 bare except.

Using bare except can catch unexpected exceptions and hide programming errors. Specify the exception type or use except Exception to catch all standard errors.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 170048a and b336e9f.

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 of if-else-block (SIM108)

Replace if-else-block with csv = 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; use if okay: for truth checks (E712)

Replace with okay


234-234: Avoid equality comparisons to True; use if okay: for truth checks (E712)

Replace with okay


242-242: Avoid equality comparisons to True; use if okay: for truth checks (E712)

Replace with okay


251-251: Avoid equality comparisons to True; use if okay: for truth checks (E712)

Replace with okay


263-263: Avoid equality comparisons to True; use if okay: for truth checks (E712)

Replace with okay


272-272: Avoid equality comparisons to True; use if okay: for truth checks (E712)

Replace with okay


279-279: Avoid equality comparisons to True; use if okay: for truth checks (E712)

Replace with okay


297-297: Avoid equality comparisons to True; use if 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; use if video.backend.grayscale: for truth checks (E712)

Replace with video.backend.grayscale


526-526: f-string without any placeholders (F541)

Remove extraneous f prefix

sleap/gui/app.py

123-123: Use super() instead of super(__class__, self) (UP008)

Remove __super__ parameters


194-194: Use super() instead of super(__class__, self) (UP008)

Remove __super__ parameters


209-210: Use a single if statement instead of nested if statements (SIM102)

Combine if statements using and


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 of for 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 of params.get("filename", None) (SIM910)

Replace params.get("filename", None) with params.get("filename")


756-756: Use params.get("video_path", None) instead of an if 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 with raise ... from err or raise ... from None to distinguish them from errors in exception handling (B904)


2101-2101: Use key in dict instead of key in dict.keys() (SIM118)

Remove .keys()


2116-2116: Use key in dict instead of key 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 of params.get("copy_instance", None) (SIM910)

Replace params.get("copy_instance", None) with params.get("copy_instance")


2889-2889: Use params.get("location") instead of params.get("location", None) (SIM910)

Replace params.get("location", None) with params.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 of DeleteFrameLimitPredictions 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 function test_DeleteFrameLimitPredictions is well-structured and tests the functionality as described in the PR. Ensure that the expected_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.

@shrivaths16 shrivaths16 requested a review from roomrys June 12, 2024 16:46
Copy link

@coderabbitai coderabbitai bot left a 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 to MainWindow.

The reference to MainWindow in the method execute of the AppCommand class is undefined within this file. Ensure that MainWindow 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 of params.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 an if 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 bare except.

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 with raise ... from err or raise ... 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: Use key in dict instead of key 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 of key 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 extraneous f 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") and params.get("location") instead of params.get("copy_instance", None) and params.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 bare except.

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

Commits

Files that changed from the base of the PR and between b336e9f and 391718b.

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 of params.get("filename", None) (SIM910)

Replace params.get("filename", None) with params.get("filename")


756-756: Use params.get("video_path", None) instead of an if 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 with raise ... from err or raise ... from None to distinguish them from errors in exception handling (B904)


2101-2101: Use key in dict instead of key in dict.keys() (SIM118)

Remove .keys()


2116-2116: Use key in dict instead of key 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 of params.get("copy_instance", None) (SIM910)

Replace params.get("copy_instance", None) with params.get("copy_instance")


2889-2889: Use params.get("location") instead of params.get("location", None) (SIM910)

Replace params.get("location", None) with params.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 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.


2476-2487: Add a comment that this is used in the InstanceDeleteCommand.ask method.

Comment on lines 2478 to 2479
"""Called from the parent `InstanceDeleteCommand.ask` class. Returns a list of
instances to be deleted."""
Copy link
Collaborator

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.

Copy link

@coderabbitai coderabbitai bot left a 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 to MainWindow.

-        """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 the get 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 the get 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 variable file_dir.

-        file_dir = os.path.dirname(filename)

Line range hint 1691-1691: Avoid using bare except 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, use raise ... from err to provide context.

-            raise e
+            raise Exception("Error during save operation.") from e

Line range hint 2101-2101: Use key in dict instead of key 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 extraneous f 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 the get 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 bare except statements. Specify exception types to handle specific exceptions appropriately.

-            except:
+            except Exception as e:
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 391718b and e16311c.

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 of params.get("filename", None) (SIM910)

Replace params.get("filename", None) with params.get("filename")


756-756: Use params.get("video_path", None) instead of an if 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 with raise ... from err or raise ... from None to distinguish them from errors in exception handling (B904)


2101-2101: Use key in dict instead of key in dict.keys() (SIM118)

Remove .keys()


2116-2116: Use key in dict instead of key 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 of params.get("copy_instance", None) (SIM910)

Replace params.get("copy_instance", None) with params.get("copy_instance")


2892-2892: Use params.get("location") instead of params.get("location", None) (SIM910)

Replace params.get("location", None) with params.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 the deleteFrameLimitPredictions 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 the InstanceDeleteCommand.ask method.

@shrivaths16 shrivaths16 requested a review from roomrys June 23, 2024 06:01
Copy link

@coderabbitai coderabbitai bot left a 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 for MainWindow.

The type hint for MainWindow is missing, causing an undefined name issue. Please import MainWindow 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 of params.get("filename", None) as None is the default return value for the get 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 with params.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 bare except statements.

Using bare except statements can catch unexpected exceptions and hide programming errors. Specify the exception type or use except 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 of key 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 is None.

- 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 bare except statements.

Replace bare except with except Exception to avoid catching unexpected exceptions.

- except:
+ except Exception:
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e16311c and 09ffa04.

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 of params.get("filename", None) (SIM910)

Replace params.get("filename", None) with params.get("filename")


756-756: Use params.get("video_path", None) instead of an if 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 with raise ... from err or raise ... from None to distinguish them from errors in exception handling (B904)


2101-2101: Use key in dict instead of key in dict.keys() (SIM118)

Remove .keys()


2116-2116: Use key in dict instead of key 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 of params.get("copy_instance", None) (SIM910)

Replace params.get("copy_instance", None) with params.get("copy_instance")


2896-2896: Use params.get("location") instead of params.get("location", None) (SIM910)

Replace params.get("location", None) with params.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 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.


2478-2479: Add a comment that this is used in the InstanceDeleteCommand.ask method.

This comment helps clarify the relationship between the DeleteFrameLimitPredictions class and its superclass, making the code easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants