Separate the video name and its filepath columns in VideoTablesModel
#2052
Conversation
WalkthroughThe changes modify the Changes
Sequence DiagramsequenceDiagram
participant User
participant Model
User->>Model: Request Video Data
Model-->>User: Return Video Data with name and filepath
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.8.2)sleap/gui/dataviews.py395-395: Undefined name (F821) 🔇 Additional comments (4)sleap/gui/dataviews.py (4)
Using
The separation of
Add proper type hint import for Video class The from pathlib import Path
from typing import Any, Callable, List, Optional
+from sleap.io.video import Video
🧰 Tools🪛 Ruff (0.8.2)395-395: Undefined name (F821)
Add error handling for None/empty filenames The current implementation might raise exceptions if def item_to_data(self, obj, item: "Video"):
data = {}
for property in self.properties:
if property == "name":
- data[property] = Path(item.filename).name
+ data[property] = Path(item.filename).name if item.filename else ""
elif property == "filepath":
- data[property] = str(Path(item.filename).parent)
+ data[property] = str(Path(item.filename).parent) if item.filename else ""
else:
data[property] = getattr(item, property)
return data
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sleap/gui/widgets/docks.py (1)
186-188: Implement or remove thecreate_context_menumethodThe
create_context_menumethod in theVideosDockclass is defined but not implemented. If it's intended for future use, consider adding aTODOcomment explaining the planned functionality. Otherwise, remove the method to keep the codebase clean.Apply this diff if you choose to remove the method:
- def create_context_menu(self) -> None: - """Create the context menu for the dock."""Or add a
TODOcomment:def create_context_menu(self) -> None: """Create the context menu for the dock.""" + # TODO: Implement context menu functionality for VideosDocksleap/io/video.py (1)
953-955: Clarify the docstring for thenamepropertyThe docstring for the
nameproperty inSingleImageVideois currently"Name of the video."Consider making it more descriptive for better clarity.Apply this diff to improve the docstring:
@property def name(self): - """Name of the video.""" + """Return the base name of the primary image file."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sleap/gui/dataviews.py(3 hunks)sleap/gui/widgets/docks.py(1 hunks)sleap/io/video.py(2 hunks)
🔇 Additional comments (7)
sleap/gui/dataviews.py (6)
294-294: Initialization of show_video_name attribute is correct
The addition of the show_video_name attribute in GenericTableView with a default value of False is appropriate and aligns with the expected behavior.
317-319: Initialization of context menu options is appropriate
The options dictionary is correctly initialized with the "Show Video Name" option, reflecting the current state of show_video_name.
393-404: Context menu implementation appears correct
The mousePressEvent method is properly overridden to handle right-click events specifically for instances of VideosTableModel. The event handling and type checking ensure that the context menu is displayed only when appropriate.
405-428: Lambda functions capture variables correctly
The create_checked_lambda method effectively creates closures that capture the option variable for each action in the context menu. This implementation avoids common pitfalls associated with lambda functions in loops.
444-467: Model properties update correctly with show_video_name toggle
The VideosTableModel class correctly updates its properties attribute based on the show_video_name flag. The set_show_video_name method ensures that changes to the flag result in the layout being updated.
470-470: Safe retrieval of item attributes in item_to_data
Using getattr(item, key, None) in the item_to_data method safely handles cases where an item may not have a specific attribute, preventing potential AttributeErrors.
sleap/io/video.py (1)
468-472: Addition of name property in MediaVideo is appropriate
The name property correctly returns the base name of the video file, enhancing usability by providing easy access to the video name.
sleap/gui/dataviews.py
Outdated
| model.show_video_name = self.show_video_name | ||
|
|
There was a problem hiding this comment.
Use setter method to update model properties
Directly assigning model.show_video_name = self.show_video_name does not update the model's properties or refresh the layout. Instead, you should use the set_show_video_name method to ensure the model updates correctly.
Apply this diff to fix the issue:
- model.show_video_name = self.show_video_name
+ model.set_show_video_name(self.show_video_name)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model.show_video_name = self.show_video_name | |
| model.set_show_video_name(self.show_video_name) | |
sleap/io/video.py
Outdated
| @property | ||
| def name(self): | ||
| """Name of the video.""" | ||
| return os.path.basename(self.filename) | ||
|
|
There was a problem hiding this comment.
Ensure filename is set when using name property
In SingleImageVideo, the name property returns os.path.basename(self.filename). However, if self.filename is None, this will raise an error. Ensure that self.filename is always set when self.filenames are provided.
Consider modifying the reset method to set self.filename when self.filenames is provided:
def reset(
self,
filename: str = None,
filenames: List[str] = None,
height_: int = None,
width_: int = None,
channels_: int = None,
grayscale: bool = None,
):
"""Reloads the video."""
if filename and filenames:
raise ValueError(
f"Cannot specify both filename and filenames for SingleImageVideo."
)
elif filename or filenames:
self.cache_ = dict()
self.test_frame_ = None
self.height_ = height_
self.width_ = width_
self.channels_ = channels_
if not filename and filenames:
self.filenames = filenames
self.filename = filenames[0]
elif filename and not filenames:
self.filename = filename
self.filenames = [filename]
+ else:
+ raise ValueError("Either filename or filenames must be specified.")Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sleap/gui/dataviews.py (1)
294-294: Initializeself.show_video_namein the__init__methodCurrently,
show_video_nameis defined as a class variable, but it's accessed asself.show_video_namewithout being explicitly initialized in the__init__method. It's best practice to initialize instance variables within the__init__method to ensure clarity and avoid unintended side effects.Apply this diff to initialize
self.show_video_namein the constructor:def __init__( self, model: QtCore.QAbstractTableModel, state: GuiState = None, row_name: Optional[str] = None, name_prefix: Optional[str] = None, is_sortable: bool = False, is_activatable: bool = False, ellipsis_left: bool = False, multiple_selection: bool = False, + show_video_name: bool = False, ): super(GenericTableView, self).__init__() + self.show_video_name = show_video_name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/dataviews.py(3 hunks)
🔇 Additional comments (3)
sleap/gui/dataviews.py (3)
309-310: Use the setter method to update show_video_name
Directly assigning model.show_video_name = self.show_video_name does not update the model's properties or refresh the layout appropriately. Instead, you should use the set_show_video_name method to ensure the model updates correctly.
Apply this diff to fix the issue:
self.setModel(model)
- model.show_video_name = self.show_video_name
+ model.set_show_video_name(self.show_video_name)393-404: Approve context menu implementation for right-click events
The implementation of mousePressEvent correctly handles right-click events to display the context menu when the model is an instance of VideosTableModel. This enhances the user experience by providing accessible options directly within the table view.
449-474: Approve dynamic property management in VideosTableModel
The VideosTableModel class effectively manages the show_video_name attribute, updating the properties based on its value and emitting layout changes appropriately. This implementation aligns with best practices for maintaining dynamic table models in Qt.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
sleap/gui/dataviews.py (3)
444-447: Simplify nested if statementsThe nested if statements can be combined using
andfor better readability.Apply this diff:
- if isinstance(model, VideosTableModel): - if option == "Show Video Name": - model.set_show_video_name(self.options["Show Video Name"]) + if isinstance(model, VideosTableModel) and option == "Show Video Name": + model.set_show_video_name(self.options["Show Video Name"])🧰 Tools
🪛 Ruff (0.8.2)
444-445: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
393-423: Consider extracting context menu creation to a separate methodThe context menu creation logic could be extracted to improve modularity and reusability.
Consider refactoring like this:
+ def create_context_menu(self) -> QtWidgets.QMenu: + """Create and return the context menu with actions. + + Returns: + QMenu: The context menu with actions. + """ + menu = QtWidgets.QMenu(self) + for option, is_checked in self.options.items(): + action = QtWidgets.QAction(option, self) + action.setCheckable(True) + action.setChecked(is_checked) + action.triggered.connect(self.create_checked_lambda(option)) + menu.addAction(action) + return menu + def show_context_menu(self, event): - menu = QtWidgets.QMenu(self) - for option, is_checked in self.options.items(): - action = QtWidgets.QAction(option, self) - action.setCheckable(True) - action.setChecked(is_checked) - action.triggered.connect(self.create_checked_lambda(option)) - menu.addAction(action) + menu = self.create_context_menu() menu.exec_(event.globalPos())
470-475: Consider caching property listsThe property lists in
_get_propertiescould be cached as class attributes to avoid creating new lists on each call.Consider refactoring like this:
+ _PROPERTIES_WITH_NAME = ["filepath", "name", "frames", "height", "width", "channels"] + _PROPERTIES_WITHOUT_NAME = ["filename", "frames", "height", "width", "channels"] + def _get_properties(self): """Return properties based on the show_video_name flag.""" - if self.show_video_name: - return ["filepath", "name", "frames", "height", "width", "channels"] - return ["filename", "frames", "height", "width", "channels"] + return self._PROPERTIES_WITH_NAME if self.show_video_name else self._PROPERTIES_WITHOUT_NAME
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/gui/dataviews.py(3 hunks)sleap/io/video.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/dataviews.py
444-445: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
487-487: Undefined name Video
(F821)
🔇 Additional comments (1)
sleap/gui/dataviews.py (1)
309-310:
Use setter method instead of direct property assignment
Direct assignment to model.show_video_name bypasses the model's property setter, which could lead to inconsistent state. Use the setter method instead.
Apply this diff:
- model.show_video_name = self.show_video_name
+ if isinstance(model, VideosTableModel):
+ model.set_show_video_name(self.show_video_name)Likely invalid or redundant comment.
sleap/gui/dataviews.py
Outdated
| def item_to_data(self, obj, item: "Video"): | ||
| data = {} | ||
| for property in self.all_properties: | ||
| data[property] = getattr(item, property) | ||
| return data |
There was a problem hiding this comment.
Add type hint for Video class
The Video type hint is undefined. Import the Video class from the appropriate module.
Add the import at the top of the file:
+from sleap.io.video import Video📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.all_properties: | |
| data[property] = getattr(item, property) | |
| return data | |
| from sleap.io.video import Video | |
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.all_properties: | |
| data[property] = getattr(item, property) | |
| return data |
🧰 Tools
🪛 Ruff (0.8.2)
487-487: Undefined name Video
(F821)
sleap/io/video.py
Outdated
| @property | ||
| def name(self): | ||
| """Name of the video.""" | ||
| return self.filename.split("/")[-1] | ||
|
|
||
| @property | ||
| def filepath(self): | ||
| """Path to the video file.""" | ||
| splitted = self.filename.split("/")[:-1] | ||
| return "/".join(splitted) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use os.path for robust path handling
The current implementation uses string splitting with "/" which may not work correctly on all platforms. Use os.path functions for more robust path handling.
Apply this diff to all video classes:
@property
def name(self):
"""Name of the video."""
- return self.filename.split("/")[-1]
+ return os.path.basename(self.filename)
@property
def filepath(self):
"""Path to the video file."""
- splitted = self.filename.split("/")[:-1]
- return "/".join(splitted)
+ return os.path.dirname(self.filename)Also applies to: 479-489, 615-625, 742-752, 991-1001
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
sleap/gui/dataviews.py (3)
444-446: Simplify nested conditionsThe nested if statements can be combined for better readability.
- if isinstance(model, VideosTableModel): - if option == "Show Video Name": + if isinstance(model, VideosTableModel) and option == "Show Video Name":🧰 Tools
🪛 Ruff (0.8.2)
444-445: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
449-449: Address the TODO comment about Pytest errorThe comment indicates an unresolved test issue that needs attention.
Would you like me to help investigate and fix the Pytest error? I can help create a new GitHub issue to track this.
496-506: Optimize attribute access and add type hintsThe code uses
getattrunnecessarily and is missing type hints.+from sleap.io.video import Video + def item_to_data(self, obj, item: Video): data = {} - for property in self.all_properties: + for property in self._all_properties: if property == "name": - data[property] = getattr(item, "filename").split("/")[-1] + data[property] = item.filename.split("/")[-1] elif property == "filepath": - splitted = getattr(item, "filename").split("/")[:-1] + splitted = item.filename.split("/")[:-1] data[property] = "/".join(splitted) else: - data[property] = getattr(item, property) + data[property] = getattr(item, property) # Keep getattr for dynamic properties return data🧰 Tools
🪛 Ruff (0.8.2)
496-496: Undefined name
Video(F821)
500-500: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
502-502: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/dataviews.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/dataviews.py
444-445: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
496-496: Undefined name Video
(F821)
500-500: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
502-502: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (2)
sleap/gui/dataviews.py (2)
309-310: Use setter method to update model properties
Direct assignment to model.show_video_name bypasses proper model updates. Use the setter method instead.
496-496: Add type hint for Video class
The Video type hint is undefined. Import the Video class from the appropriate module.
🧰 Tools
🪛 Ruff (0.8.2)
496-496: Undefined name Video
(F821)
sleap/gui/dataviews.py
Outdated
| self.properties = ( | ||
| "filepath", | ||
| "name", | ||
| "frames", | ||
| "height", | ||
| "width", | ||
| "channels", | ||
| ) | ||
| # TODO: Always with filepath and name (no filename) | ||
| self.all_properties = ( | ||
| "filename", | ||
| "filepath", | ||
| "name", | ||
| "frames", | ||
| "height", | ||
| "width", | ||
| "channels", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce property duplication
The properties are defined in multiple places (properties and all_properties), which could lead to maintenance issues.
Consider using a single source of truth:
- self.properties = (
- "filepath",
- "name",
- "frames",
- "height",
- "width",
- "channels",
- )
- # TODO: Always with filepath and name (no filename)
- self.all_properties = (
- "filename",
- "filepath",
- "name",
- "frames",
- "height",
- "width",
- "channels",
- )
+ self._all_properties = (
+ "filename",
+ "filepath",
+ "name",
+ "frames",
+ "height",
+ "width",
+ "channels",
+ )
+ self.properties = self._get_properties()Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sleap/gui/dataviews.py (1)
309-310:⚠️ Potential issueUse the setter method
set_show_video_nameto update model propertiesDirectly assigning
model.show_video_name = self.show_video_namedoes not ensure that the model updates correctly or that the view is refreshed. Instead, you should use theset_show_video_namemethod to properly update the model.Apply this diff to fix the issue:
model = self.model() - model.show_video_name = self.show_video_name + model.set_show_video_name(self.show_video_name)
🧹 Nitpick comments (1)
sleap/gui/dataviews.py (1)
449-449: Address the TODO comment in the codeThere's a
TODOcomment:# TODO: Fix the error in the Pytest.It's important to resolve this issue or create a tracking ticket before merging to ensure code quality.Would you like assistance in fixing the pytest error or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/dataviews.py(3 hunks)
🔇 Additional comments (5)
sleap/gui/dataviews.py (5)
294-294: Addition of show_video_name attribute looks good
The new attribute show_video_name is appropriately added with a default value of False in the GenericTableView class.
317-319: Refactor toggle_option to handle options dynamically
Currently, toggle_option directly references "Show Video Name", which limits scalability for adding future options. To enhance modularity and reusability, consider refactoring the method to handle different options based on the option parameter.
Apply this diff to refactor the method:
def toggle_option(self, option, checked):
"""Toggle the option in the context menu.
Args:
option (str): The option to toggle.
checked (bool): The new value for the option.
"""
self.options[option] = checked
model = self.model()
if isinstance(model, VideosTableModel):
- if option == "Show Video Name":
- model.set_show_video_name(self.options["Show Video Name"])
+ # Map options to model setter methods
+ option_setters = {
+ "Show Video Name": model.set_show_video_name,
+ # Add other options and their corresponding setter methods here
+ }
+ if option in option_setters:
+ option_setters[option](checked)Also applies to: 445-446
393-404: Context menu implementation for right-click event looks good
The mousePressEvent method is appropriately overridden to handle right-click events and display the context menu for VideosTableView.
405-447: Context menu methods are implemented correctly
The methods show_context_menu, create_checked_lambda, and toggle_option are properly implemented to handle the context menu options dynamically.
455-464: Avoid duplication by initializing properties using _get_properties method
Currently, self.properties is defined directly in __init__, and also set in the set_show_video_name method using _get_properties. To maintain consistency and reduce duplication, consider initializing self.properties in __init__ using the _get_properties method.
Apply this diff to refactor:
self.show_video_name = show_video_name
- self.properties = (
- "filepath",
- "name",
- "frames",
- "height",
- "width",
- "channels",
- )
- # TODO: Always with filepath and name (no filename)
+ self.properties = self._get_properties()
sleap/gui/dataviews.py
Outdated
| def __init__(self, items, show_video_name=False, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self.items = items | ||
| self.show_video_name = show_video_name |
There was a problem hiding this comment.
Ensure proper initialization by passing items to the superclass constructor
In the __init__ method of VideosTableModel, items is set directly with self.items = items, but it is not passed to the superclass constructor. To ensure that the superclass GenericTableModel initializes properly and handles the items correctly, consider passing items to the superclass __init__ method.
Apply this diff to fix the issue:
def __init__(self, items, show_video_name=False, **kwargs):
- super().__init__(**kwargs)
- self.items = items
+ super().__init__(items=items, **kwargs)
self.show_video_name = show_video_name📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self, items, show_video_name=False, **kwargs): | |
| super().__init__(**kwargs) | |
| self.items = items | |
| self.show_video_name = show_video_name | |
| def __init__(self, items, show_video_name=False, **kwargs): | |
| super().__init__(items=items, **kwargs) | |
| self.show_video_name = show_video_name |
sleap/gui/dataviews.py
Outdated
| def item_to_data(self, obj, item: "Video"): | ||
| data = {} | ||
| for property in self.properties: | ||
| if property == "name": | ||
| data[property] = getattr(item, "filename").split("/")[-1] | ||
| elif property == "filepath": | ||
| splitted = getattr(item, "filename").split("/")[:-1] | ||
| data[property] = "/".join(splitted) | ||
| else: | ||
| data[property] = getattr(item, property) | ||
| return data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use os.path functions for path manipulations to ensure cross-platform compatibility
Currently, the code splits the file path using split("/"), which may not work correctly on all operating systems (e.g., Windows uses backslashes). To ensure cross-platform compatibility, consider using os.path.basename and os.path.dirname for handling file paths.
Apply this diff to update the code:
import os # Ensure this import is at the top of the file
def item_to_data(self, obj, item: "Video"):
data = {}
for property in self.properties:
if property == "name":
- data[property] = getattr(item, "filename").split("/")[-1]
+ data[property] = os.path.basename(item.filename)
elif property == "filepath":
- splitted = getattr(item, "filename").split("/")[:-1]
- data[property] = "/".join(splitted)
+ data[property] = os.path.dirname(item.filename)
else:
data[property] = getattr(item, property)
return dataThis change ensures that file path manipulations are handled correctly across different operating systems.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.properties: | |
| if property == "name": | |
| data[property] = getattr(item, "filename").split("/")[-1] | |
| elif property == "filepath": | |
| splitted = getattr(item, "filename").split("/")[:-1] | |
| data[property] = "/".join(splitted) | |
| else: | |
| data[property] = getattr(item, property) | |
| return data | |
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.properties: | |
| if property == "name": | |
| data[property] = os.path.basename(item.filename) | |
| elif property == "filepath": | |
| data[property] = os.path.dirname(item.filename) | |
| else: | |
| data[property] = getattr(item, property) | |
| return data |
7174Andy
changed the title
VideoTablesModelVideoTablesModel
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sleap/gui/dataviews.py (1)
398-398: Add type hint for Video classImport the Video class to properly type hint the item parameter:
+from sleap.io.video import Video🧰 Tools
🪛 Ruff (0.8.2)
398-398: Undefined name
Video(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/dataviews.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/dataviews.py
398-398: Undefined name Video
(F821)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
404-404: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (2)
sleap/gui/dataviews.py (2)
389-396: LGTM! Properties updated to separate filename into name and filepath.
The properties tuple has been correctly updated to include separate columns for the video name and filepath, which aligns with the PR objective to improve user experience.
398-408:
Use os.path for cross-platform path handling and improve error handling
- Replace string splitting with os.path functions for cross-platform compatibility
- Add error handling for missing filename attribute
- Simplify attribute access by removing unnecessary getattr calls
Apply this diff to fix the issues:
def item_to_data(self, obj, item: "Video"):
data = {}
for property in self.properties:
- if property == "name":
- data[property] = getattr(item, "filename").split("/")[-1]
- elif property == "filepath":
- splitted = getattr(item, "filename").split("/")[:-1]
- data[property] = "/".join(splitted)
- else:
- data[property] = getattr(item, property)
+ try:
+ if property == "name":
+ data[property] = os.path.basename(item.filename)
+ elif property == "filepath":
+ data[property] = os.path.dirname(item.filename)
+ else:
+ data[property] = getattr(item, property)
+ except AttributeError as e:
+ data[property] = "" # or None, depending on your preference
return dataLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
398-398: Undefined name Video
(F821)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
404-404: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2052 +/- ##
===========================================
+ Coverage 73.30% 75.43% +2.12%
===========================================
Files 134 134
Lines 24087 24750 +663
===========================================
+ Hits 17658 18670 +1012
+ Misses 6429 6080 -349 ☔ View full report in Codecov by Sentry. |
sleap/gui/dataviews.py
Outdated
| return ["filepath", "name", "frames", "height", "width", "channels"] | ||
| return ["filename", "frames", "height", "width", "channels"] |
There was a problem hiding this comment.
Do we want "filename" or "name" to be the column name?
Co-authored-by: Liezl Maree <[email protected]>
Co-authored-by: Liezl Maree <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/dataviews.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/dataviews.py
398-398: Undefined name Video
(F821)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
403-403: Local variable name is assigned to but never used
Remove assignment to unused variable name
(F841)
403-403: Undefined name Path
(F821)
405-405: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
406-406: Local variable parent is assigned to but never used
Remove assignment to unused variable parent
(F841)
406-406: Undefined name Path
(F821)
407-407: Undefined name splitted
(F821)
🔇 Additional comments (2)
sleap/gui/dataviews.py (2)
389-396: LGTM! Properties declaration looks good.
The separation of filename into name and filepath properties is well-structured and maintains a logical order.
398-410: Consider standardizing path handling across the application.
Since we're now using pathlib.Path for path operations in this class, it might be worth standardizing this approach across the application for consistency and better cross-platform compatibility.
Let's check the current path handling approaches in the codebase:
🧰 Tools
🪛 Ruff (0.8.2)
398-398: Undefined name Video
(F821)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
403-403: Local variable name is assigned to but never used
Remove assignment to unused variable name
(F841)
403-403: Undefined name Path
(F821)
405-405: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
406-406: Local variable parent is assigned to but never used
Remove assignment to unused variable parent
(F841)
406-406: Undefined name Path
(F821)
407-407: Undefined name splitted
(F821)
sleap/gui/dataviews.py
Outdated
| def item_to_data(self, obj, item: "Video"): | ||
| data = {} | ||
| for property in self.properties: | ||
| if property == "name": | ||
| filename = getattr(item, "filename") | ||
| name = Path(filename).name | ||
| elif property == "filepath": | ||
| filename = getattr(item, "filename") | ||
| parent = Path(filename).parent | ||
| data[property] = "/".join(splitted) | ||
| else: | ||
| data[property] = getattr(item, property) | ||
| return data |
There was a problem hiding this comment.
Fix critical issues in the item_to_data method implementation.
The current implementation has several issues:
- The extracted
nameandparentvalues are not used - The undefined
splittedvariable is used - Missing required imports
- No handling for potential None values
Apply this diff to fix the issues:
+from pathlib import Path
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from sleap.io.video import Video
def item_to_data(self, obj, item: "Video"):
data = {}
for property in self.properties:
if property == "name":
- filename = getattr(item, "filename")
- name = Path(filename).name
+ filename = item.filename
+ data[property] = Path(filename).name if filename else ""
elif property == "filepath":
- filename = getattr(item, "filename")
- parent = Path(filename).parent
- data[property] = "/".join(splitted)
+ filename = item.filename
+ data[property] = str(Path(filename).parent) if filename else ""
else:
data[property] = getattr(item, property)
return dataChanges made:
- Added required imports
- Properly used Path for name and filepath extraction
- Added null checks for filename
- Removed unnecessary
getattrcalls - Fixed the filepath construction using
str(Path().parent)
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.properties: | |
| if property == "name": | |
| filename = getattr(item, "filename") | |
| name = Path(filename).name | |
| elif property == "filepath": | |
| filename = getattr(item, "filename") | |
| parent = Path(filename).parent | |
| data[property] = "/".join(splitted) | |
| else: | |
| data[property] = getattr(item, property) | |
| return data | |
| from pathlib import Path | |
| from typing import TYPE_CHECKING | |
| if TYPE_CHECKING: | |
| from sleap.io.video import Video | |
| def item_to_data(self, obj, item: "Video"): | |
| data = {} | |
| for property in self.properties: | |
| if property == "name": | |
| filename = item.filename | |
| data[property] = Path(filename).name if filename else "" | |
| elif property == "filepath": | |
| filename = item.filename | |
| data[property] = str(Path(filename).parent) if filename else "" | |
| else: | |
| data[property] = getattr(item, property) | |
| return data |
🧰 Tools
🪛 Ruff (0.8.2)
398-398: Undefined name Video
(F821)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
403-403: Local variable name is assigned to but never used
Remove assignment to unused variable name
(F841)
403-403: Undefined name Path
(F821)
405-405: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
406-406: Local variable parent is assigned to but never used
Remove assignment to unused variable parent
(F841)
406-406: Undefined name Path
(F821)
407-407: Undefined name splitted
(F821)
Co-authored-by: Liezl Maree <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sleap/gui/dataviews.py (1)
401-410: Simplify attribute access and improve code conciseness.The current implementation can be improved by:
- Using direct attribute access instead of
getattr- Making the code more concise
def item_to_data(self, obj, item: "Video"): data = {} for property in self.properties: if property == "name": - filename = getattr(item, "filename") - data[property] = pathlib.Path(filename).name + data[property] = pathlib.Path(item.filename).name elif property == "filepath": - filename = getattr(item, "filename") - data[property] = str(pathlib.Path(filename).parent) + data[property] = str(pathlib.Path(item.filename).parent) else: - data[property] = getattr(item, property) + data[property] = getattr(item, property) # Keep getattr here for dynamic property access return data🧰 Tools
🪛 Ruff (0.8.2)
403-403: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
406-406: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/dataviews.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/dataviews.py
399-399: Undefined name Video
(F821)
403-403: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
406-406: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (2)
sleap/gui/dataviews.py (2)
390-397: LGTM! Properties updated to separate filename into name and filepath.
The property names are clear and follow consistent naming conventions.
399-399:
Add import for Video type hint.
The Video type hint is undefined. Add the following import:
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from sleap.io.video import VideoLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
399-399: Undefined name Video
(F821)
and sort imports and remove unused imports
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/dataviews.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/dataviews.py
395-395: Undefined name Video
(F821)
399-399: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (4)
sleap/gui/dataviews.py (4)
20-20: LGTM!
The addition of pathlib.Path import is appropriate for cross-platform path handling.
386-393: LGTM!
The properties tuple correctly separates the filename into name and filepath components, improving clarity in the table view.
386-406: Verify handling of special cases
Please verify the following scenarios:
- Videos with special characters in their paths
- Network paths (if applicable)
- Impact on existing video table views
✅ Verification successful
Let me analyze the results and generate a final response.
No issues found with path handling
The code changes in dataviews.py are safe and properly handle paths:
-
Special characters in paths: The code uses Python's
Pathclass which handles special characters in paths correctly across platforms. -
Network paths: The code only extracts the parent directory and filename components using
Pathclass methods which work correctly with both local and network paths (e.g., UNC paths with double slashes). -
Impact on video table views: The changes only affect display formatting and don't modify any underlying path handling logic.
The implementation:
- Uses
Path(filename).nameto safely extract filename - Uses
Path(filename).parentto safely get parent directory - Relies on Python's built-in path handling which is well-tested for edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential path edge cases in the codebase
# Look for existing video paths with special characters
rg -g '!*.pyc' -g '!*.git' 'filename.*[^a-zA-Z0-9/\._-]'
# Check for network paths
rg -g '!*.pyc' -g '!*.git' 'filename.*[/\\]{2}'
# Look for existing video table view usage
ast-grep --pattern 'VideosTableModel($$$)'
Length of output: 67973
🧰 Tools
🪛 Ruff (0.8.2)
395-395: Undefined name Video
(F821)
399-399: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
395-395:
Add import for Video type hint
The Video type hint is undefined. Add the following import:
from typing import Any, Callable, List, Optional
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from sleap.io.video import VideoLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
395-395: Undefined name Video
(F821)
sleap/gui/dataviews.py
Outdated
| if property == "name": | ||
| filename = getattr(item, "filename") | ||
| data[property] = Path(filename).name | ||
| elif property == "filepath": | ||
| filename = getattr(item, "filename") | ||
| data[property] = str(Path(filename).parent) | ||
| else: | ||
| data[property] = getattr(item, property) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify attribute access and add error handling
The current implementation has several areas for improvement:
- Using
getattris unnecessary for constant attributes - Missing error handling for None/empty filenames
- Code could be more concise
Apply this diff to improve the implementation:
if property == "name":
- filename = getattr(item, "filename")
- data[property] = Path(filename).name
+ filename = item.filename
+ data[property] = Path(filename).name if filename else ""
elif property == "filepath":
- filename = getattr(item, "filename")
- data[property] = str(Path(filename).parent)
+ filename = item.filename
+ data[property] = str(Path(filename).parent) if filename else ""
else:
- data[property] = getattr(item, property)
+ data[property] = getattr(item, property, "")📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if property == "name": | |
| filename = getattr(item, "filename") | |
| data[property] = Path(filename).name | |
| elif property == "filepath": | |
| filename = getattr(item, "filename") | |
| data[property] = str(Path(filename).parent) | |
| else: | |
| data[property] = getattr(item, property) | |
| if property == "name": | |
| filename = item.filename | |
| data[property] = Path(filename).name if filename else "" | |
| elif property == "filepath": | |
| filename = item.filename | |
| data[property] = str(Path(filename).parent) if filename else "" | |
| else: | |
| data[property] = getattr(item, property, "") |
🧰 Tools
🪛 Ruff (0.8.2)
399-399: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
402-402: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
* Remove no-op code from #1498 * Add options to set background color when exporting video (#1328) * implement #921 * simplified form / refractor * Add test function and update cli docs * Improve test function to check background color * Improve comments * Change background options to lowercase * Use coderabbitai suggested `fill` --------- Co-authored-by: Shrivaths Shyam <[email protected]> Co-authored-by: Liezl Maree <[email protected]> * Increase range on batch size (#1513) * Increase range on batch size * Set maximum to a factor of 2 * Set default callable for `match_lists_function` (#1520) * Set default for `match_lists_function` * Move test code to official tests * Check using expected values * Allow passing in `Labels` to `app.main` (#1524) * Allow passing in `Labels` to `app.main` * Load the labels object through command * Add warning when unable to switch back to CPU mode * Replace (broken) `--unrag` with `--ragged` (#1539) * Fix unrag always set to true in sleap-export * Replace unrag with ragged * Fix typos * Add function to create app (#1546) * Refactor `AddInstance` command (#1561) * Refactor AddInstance command * Add staticmethod wrappers * Return early from set_visible_nodes * Import DLC with uniquebodyparts, add Tracks (#1562) * Import DLC with uniquebodyparts, add Tracks * add tests * correct tests * Make the hdf5 videos store as int8 format (#1559) * make the hdf5 video dataset type as proper int8 by padding with zeros * add gzip compression * Scale new instances to new frame size (#1568) * Fix typehinting in `AddInstance` * brought over changes from my own branch * added suggestions * Ensured google style comments --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: sidharth srinath <[email protected]> * Fix package export (#1619) Add check for empty videos * Add resize/scroll to training GUI (#1565) * Make resizable training GUI and add adaptive scroll bar * Set a maximum window size --------- Co-authored-by: Liezl Maree <[email protected]> * support loading slp files with non-compound types and str in metadata (#1566) Co-authored-by: Liezl Maree <[email protected]> * change inference pipeline option to tracking-only (#1666) change inference pipeline none option to tracking-only * Add ABL:AOC 2023 Workshop link (#1673) * Add ABL:AOC 2023 Workshop link * Trigger website build * Graceful failing with seeking errors (#1712) * Don't try to seek to faulty last frame on provider initialization * Catch seeking errors and pass * Lint * Fix IndexError for hdf5 file import for single instance analysis files (#1695) * Fix hdf5 read for single instance analysis files * Add test * Small test files * removing unneccessary fixtures * Replace imgaug with albumentations (#1623) What's the worst that could happen? * Initial commit * Fix augmentation * Update more deps requirements * Use pip for installing albumentations and avoid reinstalling OpenCV * Update other conda envs * Fix out of bounds albumentations issues and update dependencies (#1724) * Install albumentations using conda-forge in environment file * Conda install albumentations * Add ndx-pose to pypi requirements * Keep out of bounds points * Black * Add ndx-pose to conda install in environment file * Match environment file without cuda * Ordered dependencies * Add test * Delete comments * Add conda packages to mac environment file * Order dependencies in pypi requirements * Add tests with zeroes and NaNs for augmentation * Back * Black * Make comment one line * Add todo for later * Black * Update to new TensorFlow conda package (#1726) * Build conda package locally * Try 2.8.4 * Merge develop into branch to fix dependencies * Change tensorflow version to 2.7.4 in where conda packages are used * Make tensorflow requirements in pypi looser * Conda package has TensorFlow 2.7.0 and h5py and numpy installed via conda * Change tensorflow version in `environment_no_cuda.yml` to test using CI * Test new sleap/tensorflow package * Reset build number * Bump version * Update mac deps * Update to Arm64 Mac runners * pin `importlib-metadata` * Pin more stuff on mac * constrain `opencv` version due to new qt dependencies * Update more mac stuff * Patches to get to green * More mac skipping --------- Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> * Fix CI on macosx-arm64 (#1734) * Build conda package locally * Try 2.8.4 * Merge develop into branch to fix dependencies * Change tensorflow version to 2.7.4 in where conda packages are used * Make tensorflow requirements in pypi looser * Conda package has TensorFlow 2.7.0 and h5py and numpy installed via conda * Change tensorflow version in `environment_no_cuda.yml` to test using CI * Test new sleap/tensorflow package * Reset build number * Bump version * Update mac deps * Update to Arm64 Mac runners * pin `importlib-metadata` * Pin more stuff on mac * constrain `opencv` version due to new qt dependencies * Update more mac stuff * Patches to get to green * More mac skipping * Re-enable mac tests * Handle GPU re-init * Fix mac build CI * Widen tolerance for movenet correctness test * Fix build ci * Try for manual build without upload * Try to reduce training CI time * Rework actions * Fix miniforge usage * Tweaks * Fix build ci * Disable manual build * Try merging CI coverage * GPU/CPU usage in tests * Lint * Clean up * Fix test skip condition * Remove scratch test --------- Co-authored-by: eberrigan <[email protected]> * Add option to export to CSV via sleap-convert and API (#1730) * Add csv as a format option * Add analysis to format * Add csv suffix to output path * Add condition for csv analysis file * Add export function to Labels class * delete print statement * lint * Add `analysis.csv` as parametrize input for `sleap-convert` tests * test `export_csv` method added to `Labels` class * black formatting * use `Path` to construct filename * add `analysis.csv` to cli guide for `sleap-convert` --------- Co-authored-by: Talmo Pereira <[email protected]> * Only propagate Transpose Tracks when propagate is checked (#1748) Fix always-propagate transpose tracks issue * View Hyperparameter nonetype fix (#1766) Pass config getter argument to fetch hyperparameters * Adding ragged metadata to `info.json` (#1765) Add ragged metadata to info.json file * Add batch size to GUI for inference (#1771) * Fix conda builds (#1776) * test conda packages in a test environment as part of CI * do not test sleap import using conda build * use github environment variables to define build path for each OS in the matrix and add print statements for testing * figure out paths one OS at a time * github environment variables work in subsequent steps not current step * use local builds first * print env info * try simple environment creation * try conda instead of mamba * fix windows build path * fix windows build path * add comment to reference pull request * remove test stage from conda build for macs and test instead by creating the environment in a workflow * test workflow by pushing to current branch * test conda package on macos runner * Mac build does not need nvidia channel * qudida and albumentations are conda installed now * add comment with original issue * use python 3.9 * use conda match specifications syntax * make print statements more readable for troubleshooting python versioning * clean up build file * update version for pre-release * add TODO * add tests for conda packages before uploading * update ci comments and branches * remove macos test of pip wheel since python 3.9 is not supported by setup-python action * Upgrade build actions for release (#1779) * update `build.yml` so it matches updates from `build_manual.yml` * test `build.yml` without uploading * test again using build_manual.yml * build pip wheel with Ubuntu and turn off caching so build.yml exactly matches build_manual.yml * `build.yml` on release only and upload * testing caching * `use-only-tar-bz2: true` makes environment unsolvable, change it back * Update .github/workflows/build_manual.yml Co-authored-by: Liezl Maree <[email protected]> * Update .github/workflows/build.yml Co-authored-by: Liezl Maree <[email protected]> * bump pre-release version * fix version for pre-release * run build and upload on release! * try setting `CACHE_NUMBER` to 1 with `use-only-tar-bz2` set to true * increasing the cache number to reset the cache does work when `use-only-tar-bz2` is set to true * publish and upload on release only --------- Co-authored-by: Liezl Maree <[email protected]> * Add ZMQ support via GUI and CLI (#1780) * Add ZMQ support via GUI and CLI, automatic port handler, separate utils module for the functions * Change menu name to match deleting predictions beyond max instance (#1790) Change menu and function names * Fix website build and remove build cache across workflows (#1786) * test with build_manual on push * comment out caching in build manual * remove cache step from builad manual since environment resolves when this is commented out * comment out cache in build ci * remove cache from build on release * remove cache from website build * test website build on push * add name to checkout step * update checkout to v4 * update checkout to v4 in build ci * remove cache since build ci works without it * update upload-artifact to v4 in build ci * update second chechout to v4 in build ci * update setup-python to v5 in build ci * update download-artifact to v4 in build ci * update checkout to v4 in build ci * update checkout to v4 in website build * update setup-miniconda to v3.0.3 in website build * update actions-gh-pages to v4 in website build * update actions checkout and setup-python in ci * update checkout action in ci to v4 * pip install lxml[html_clean] because of error message during action * add error message to website to explain why pip install lxml[html_clean] * remove my branch for pull request * Bump to 1.4.1a1 (#1791) * bump versions to 1.4.1a1 * we can change the version on the installation page since this will be merged into the develop branch and not main * Fix windows conda package upload and build ci (#1792) * windows OS is 2022 not 2019 on runner * upload windows conda build manually but not pypi build * remove comment and run build ci * change build manual back so that it doesn't upload * remove branch from build manual * update installation docs for 1.4.1a1 * Fix zmq inference (#1800) * Ensure that we always pass in the zmq_port dict to LossViewer * Ensure zmq_ports has correct keys inside LossViewer * Use specified controller and publish ports for first attempted addresses * Add test for ports being set in LossViewer * Add max attempts to find unused port * Fix find free port loop and add for controller port also * Improve code readablility and reuse * Improve error message when unable to find free port * Set selected instance to None after removal (#1808) * Add test that selected instance set to None after removal * Set selected instance to None after removal * Add `InstancesList` class to handle backref to `LabeledFrame` (#1807) * Add InstancesList class to handle backref to LabeledFrame * Register structure/unstructure hooks for InstancesList * Add tests for the InstanceList class * Handle case where instance are passed in but labeled_frame is None * Add tests relevant methods in LabeledFrame * Delegate setting frame to InstancesList * Add test for PredictedInstance.frame after complex merge * Add todo comment to not use Instance.frame * Add rtest for InstasnceList.remove * Use normal list for informative `merged_instances` * Add test for copy and clear * Add copy and clear methods, use normal lists in merge method * Bump to v1.4.1a2 (#1835) bump to 1.4.1a2 * Updated trail length viewing options (#1822) * updated trail length optptions * Updated trail length options in the view menu * Updated `prefs` to include length info from `preferences.yaml` * Added trail length as method of `MainWindow` * Updated trail length documentation * black formatting --------- Co-authored-by: Keya Loding <[email protected]> * Handle case when no frame selection for trail overlay (#1832) * Menu option to open preferences directory and update to util functions to pathlib (#1843) * Add menu to view preferences directory and update to pathlib * text formatting * Add `Keep visualizations` checkbox to training GUI (#1824) * Renamed save_visualizations to view_visualizations for clarity * Added Delete Visualizations button to the training pipeline gui, exposed del_viz_predictions config option to the user * Reverted view_ back to save_ and changed new training checkbox to Keep visualization images after training. * Fixed keep_viz config option state override bug and updated keep_viz doc description * Added test case for reading training CLI argument correctly * Removed unnecessary testing code * Creating test case to check for viz folder * Finished tests to check CLI argument reading and viz directory existence * Use empty string instead of None in cli args test * Use keep_viz_images false in most all test configs (except test to override config) --------- Co-authored-by: roomrys <[email protected]> * Allowing inference on multiple videos via `sleap-track` (#1784) * implementing proposed code changes from issue #1777 * comments * configuring output_path to support multiple video inputs * fixing errors from preexisting test cases * Test case / code fixes * extending test cases for mp4 folders * test case for output directory * black and code rabbit fixes * code rabbit fixes * as_posix errors resolved * syntax error * adding test data * black * output error resolved * edited for push to dev branch * black * errors fixed, test cases implemented * invalid output test and invalid input test * deleting debugging statements * deleting print statements * black * deleting unnecessary test case * implemented tmpdir * deleting extraneous file * fixing broken test case * fixing test_sleap_track_invalid_output * removing support for multiple slp files * implementing talmo's comments * adding comments * Add object keypoint similarity method (#1003) * Add object keypoint similarity method * fix max_tracking * correct off-by-one error * correct off-by-one error * Generate suggestions using max point displacement threshold (#1862) * create function max_point_displacement, _max_point_displacement_video. Add to yaml file. Create test for new function . . . will need to edit * remove unnecessary for loop, calculate proper displacement, adjusted tests accordingly * Increase range for displacement threshold * Fix frames not found bug * Return the latter frame index * Lint --------- Co-authored-by: roomrys <[email protected]> * Added Three Different Cases for Adding a New Instance (#1859) * implemented paste with offset * right click and then default will paste the new instance at the location of the cursor * modified the logics for creating new instance * refined the logic * fixed the logic for right click * refined logics for adding new instance at a specific location * Remove print statements * Comment code * Ensure that we choose a non nan reference node * Move OOB nodes to closest in-bounds position --------- Co-authored-by: roomrys <[email protected]> * Allow csv and text file support on sleap track (#1875) * initial changes * csv support and test case * increased code coverage * Error fixing, black, deletion of (self-written) unused code * final edits * black * documentation changes * documentation changes * Fix GUI crash on scroll (#1883) * Only pass wheelEvent to children that can handle it * Add test for wheelEvent * Fix typo to allow rendering videos with mp4 (Mac) (#1892) Fix typo to allow rendering videos with mp4 * Do not apply offset when double clicking a `PredictedInstance` (#1888) * Add offset argument to newInstance and AddInstance * Apply offset of 10 for Add Instance menu button (Ctrl + I) * Add offset for docks Add Instance button * Make the QtVideoPlayer context menu unit-testable * Add test for creating a new instance * Add test for "New Instance" button in `InstancesDock` * Fix typo in docstring * Add docstrings and typehinting * Remove unused imports and sort imports * Refactor video writer to use imageio instead of skvideo (#1900) * modify `VideoWriter` to use imageio with ffmpeg backend * check to see if ffmpeg is present * use the new check for ffmpeg * import imageio.v2 * add imageio-ffmpeg to environments to test * using avi format for now * remove SKvideo videowriter * test `VideoWriterImageio` minimally * add more documentation for ffmpeg * default mp4 for ffmpeg should be mp4 * print using `IMAGEIO` when using ffmpeg * mp4 for ffmpeg * use mp4 ending in test * test `VideoWriterImageio` with avi file extension * test video with odd size * remove redundant filter since imageio-ffmpeg resizes automatically * black * remove unused import * use logging instead of print statement * import cv2 is needed for resize * remove logging * Use `Video.from_filename` when structuring videos (#1905) * Use Video.from_filename when structuring videos * Modify removal_test_labels to have extension in filename * Use | instead of + in key commands (#1907) * Use | instead of + in key commands * Lint * Replace QtDesktop widget in preparation for PySide6 (#1908) * Replace to-be-depreciated QDesktopWidget * Remove unused imports and sort remaining imports * Remove unsupported |= operand to prepare for PySide6 (#1910) Fixes TypeError: unsupported operand type(s) for |=: 'int' and 'Option' * Use positional argument for exception type (#1912) traceback.format_exception has changed it's first positional argument's name from etype to exc in python 3.7 to 3.10 * Replace all Video structuring with Video.cattr() (#1911) * Remove unused AsyncVideo class (#1917) Remove unused AsyncVideo * Refactor `LossViewer` to use matplotlib (#1899) * use updated syntax for QtAgg backend of matplotlib * start add features to `MplCanvas` to replace QtCharts features in `LossViewer` (untested) * remove QtCharts imports and replace with MplCanvas * remove QtCharts imports and replace with MplCanvas * start using MplCanvas in LossViwer instead of QtCharts (untested) * use updated syntax * Uncomment all commented out QtChart * Add debug code * Refactor monitor to use LossViewer._init_series method * Add monitor only debug code * Add methods for setting up axes and legend * Add the matplotlib canvas to the widget * Resize axis with data (no log support yet) * Try using PathCollection for "batch" * Get "batch" plotting with ax.scatter (no log support yet) * Add log support * Add a _resize_axis method * Modify init_series to work for ax.plot as well * Use matplotlib to plot epoch_loss line * Add method _add_data_to_scatter * Add _add_data_to_plot method * Add docstring to _resize_axes * Add matplotlib plot for val_loss * Add matplotlib scatter for val_loss_best * Avoid errors with setting log scale before any positive values * Add x and y axes labels * Set title (removing html tags) * Add legend * Adjust positioning of plot * Lint * Leave MplCanvas unchanged * Removed unused training_monitor.LossViewer * Resize fonts * Move legend outside of plot * Add debug code for montitor aesthetics * Use latex formatting to bold parts of title * Make axes aesthetic * Add midpoint grid lines * Set initial limits on x and y axes to be 0+ * Ensure x axis minimum is always resized to 0+ * Adjust plot to account for plateau patience title * Add debug code for plateau patience title line * Lint * Set thicker line width * Remove unused import * Set log axis on initialization * Make tick labels smaller * Move plot down a smidge * Move ylabel left a bit * Lint * Add class LossPlot * Refactor LossViewer to use LossPlot * Remove QtCharts code * Remove debug codes * Allocate space for figure items based on item's size * Refactor LossPlot to use underscores for internal methods * Ensure y_min, y_max not equal Otherwise we get an unnecessary teminal message: UserWarning: Attempting to set identical bottom == top == 3.0 results in singular transformations; automatically expanding. self.axes.set_ylim(y_min, y_max) --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: roomrys <[email protected]> * Refactor `LossViewer` to use underscores for internal method names (#1919) Refactor LossViewer to use underscores for internal method names * Manually handle `Instance.from_predicted` structuring when not `None` (#1930) * Use `tf.math.mod` instead of `%` (#1931) * Option for Max Stride to be 128 (#1941) Co-authored-by: Max Weinberg <[email protected]> * Add discussion comment workflow (#1945) * Add a bot to autocomment on workflow * Use github markdown warning syntax * Add a multiline warning * Change happy coding to happy SLEAPing Co-authored-by: Talmo Pereira <[email protected]> --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> * Add comment on issue workflow (#1946) * Add workflow to test conda packages (#1935) * Add missing imageio-ffmpeg to meta.ymls (#1943) * Update installation docs 1.4.1 (#1810) * [wip] Updated installation docs * Add tabs for different OS installations * Move installation methods to tabs * Use tabs.css * FIx styling error (line under last tab in terminal hint) * Add installation instructions before TOC * Replace mamba with conda * Lint * Find good light colors not switching when change dark/light themes * Get color scheme switching with dark/light toggle button * Upgrade website build dependencies * Remove seemingly unneeded dependencies from workflow * Add myst-nb>=0.16.0 lower bound * Trigger dev website build * Fix minor typo in css * Add miniforge and one-liner installs for package managers --------- Co-authored-by: roomrys <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> * Add imageio dependencies for pypi wheel (#1950) Add imagio dependencies for pypi wheel Co-authored-by: roomrys <[email protected]> * Do not always color skeletons table black (#1952) Co-authored-by: roomrys <[email protected]> * Remove no module named work error (#1956) * Do not always color skeletons table black * Remove offending (possibly unneeded) line that causes the no module named work error to print in terminal * Remove offending (possibly unneeded) line that causes the no module named work error to print in terminal * Remove accidentally added changes * Add (failing) test to ensure menu-item updates with state change * Reconnect callback for menu-item (using lambda) * Add (failing) test to ensure menu-item updates with state change Do not assume inital state * Reconnect callback for menu-item (using lambda) --------- Co-authored-by: roomrys <[email protected]> * Add `normalized_instance_similarity` method (#1939) * Add normalize function * Expose normalization function * Fix tests * Expose object keypoint sim function * Fix tests * Handle skeleton decoding internally (#1961) * Reorganize (and add) imports * Add (and reorganize) imports * Modify decode_preview_image to return bytes if specified * Implement (minimally tested) replace_jsonpickle_decode * Add support for using idx_to_node map i.e. loading from Labels (slp file) * Ignore None items in reduce_list * Convert large function to SkeletonDecoder class * Update SkeletonDecoder.decode docstring * Move decode_preview_image to SkeletonDecoder * Use SkeletonDecoder instead of jsonpickle in tests * Remove unused imports * Add test for decoding dict vs tuple pystates * Handle skeleton encoding internally (#1970) * start class `SkeletonEncoder` * _encoded_objects need to be a dict to add to * add notebook for testing * format * fix type in docstring * finish classmethod for encoding Skeleton as a json string * test encoded Skeleton as json string by decoding it * add test for decoded encoded skeleton * update jupyter notebook for easy testing * constraining attrs in dev environment to make sure decode format is always the same locally * encode links first then encode source then target then type * save first enconding statically as an input to _get_or_assign_id so that we do not always get py/id * save first encoding statically * first encoding is passed to _get_or_assign_id * use first_encoding variable to determine if we should assign a py/id * add print statements for debugging * update notebook for easy testing * black * remove comment * adding attrs constraint to show this passes for certain attrs version only * add import * switch out jsonpickle.encode * oops remove import * can attrs be unconstrained? * forgot comma * pin attrs for testing * test Skeleton from json, template, with symmetries, and template * use SkeletonEncoder.encode * black * try removing None values in EdgeType reduced * Handle case when nodes are replaced by integer indices from caller * Remove prototyping notebook * Remove attrs pins * Remove sort keys (which flips the neccessary ordering of our py/ids) * Do not add extra indents to encoded file * Only append links after fully encoded (fat-finger) * Remove outdated comment * Lint --------- Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: roomrys <[email protected]> * Pin ndx-pose<0.2.0 (#1978) * Pin ndx-pose<0.2.0 * Typo * Sort encoded `Skeleton` dictionary for backwards compatibility (#1975) * Add failing test to check that encoded Skeleton is sorted * Sort Skeleton dictionary before encoding * Remove unused import * Disable comment bot for now * Fix COCO Dataset Loading for Invisible Keypoints (#2035) Update coco.py # Fix COCO Dataset Loading for Invisible Keypoints ## Issue When loading COCO datasets, keypoints marked as invisible (flag=0) are currently skipped and later placed randomly within the instance's bounding box. However, in COCO format, these keypoints may still have valid coordinate information that should be preserved (see toy_dataset for expected vs. current behavior). ## Changes Modified the COCO dataset loading logic to: - Check if invisible keypoints (flag=0) have non-zero coordinates - If coordinates are (0,0), skip the point (existing behavior) - If coordinates are not (0,0), create the point at those coordinates but mark it as not visible - Maintain existing behavior for visible (flag=2) and labeled * Lint * Add tracking score as seekbar header options (#2047) * Add `tracking_score` as a constructor arg for `PredictedInstance` * Add `tracking_score` to ID models * Add fixture with tracking scores * Add tracking score to seekbar header * Add bonsai guide for sleap docs (#2050) * [WIP] Add bonsai guide page * Add more information to the guide with images * add branch for website build * Typos * fix links * Include suggestions * Add more screenshots and refine the doc * Remove branch from website workflow * Completed documentation edits from PR made by reviewer + review bot. --------- Co-authored-by: Shrivaths Shyam <[email protected]> Co-authored-by: Liezl Maree <[email protected]> * Don't mark complete on instance scaling (#2049) * Add check for instances with track assigned before training ID models (#2053) * Add menu item for deleting instances beyond frame limit (#1797) * Add menu item for deleting instances beyond frame limit * Add test function to test the instances returned * typos * Update docstring * Add frame range form * Extend command to use frame range --------- Co-authored-by: Talmo Pereira <[email protected]> * Highlight instance box on hover (#2055) * Make node marker and label sizes configurable via preferences (#2057) * Make node marker and label sizes configurable via preferences * Fix test * Enable touchpad pinch to zoom (#2058) * Fix import PySide2 -> qtpy (#2065) * Fix import PySide2 -> qtpy * Remove unnecessary print statements. * Add channels for pip conda env (#2067) * Add channels for pypi conda env * Trigger dev website build * Separate the video name and its filepath columns in `VideoTablesModel` (#2052) * add option to show video names with filepath * add doc * new feature added successfully * delete unnecessary code * remove attributes from video object * Update dataviews.py * remove all properties * delete toggle option * remove video show * fix the order of the columns * remove options * Update sleap/gui/dataviews.py Co-authored-by: Liezl Maree <[email protected]> * Update sleap/gui/dataviews.py Co-authored-by: Liezl Maree <[email protected]> * use pathlib instead of substrings * Update dataviews.py Co-authored-by: Liezl Maree <[email protected]> * Use Path instead of pathlib.Path and sort imports and remove unused imports * Use item.filename instead of getattr --------- Co-authored-by: Liezl Maree <[email protected]> * Make status bar dependent on UI mode (#2063) * remove bug for dark mode * fix toggle case --------- Co-authored-by: Liezl Maree <[email protected]> * Bump version to 1.4.1 (#2062) * Bump version to 1.4.1 * Trigger conda/pypi builds (no upload) * Trigger website build * Add dev channel to installation instructions --------- Co-authored-by: Talmo Pereira <[email protected]> * Add -c sleap/label/dev channel for win/linux - also trigger website build --------- Co-authored-by: Scott Yang <[email protected]> Co-authored-by: Shrivaths Shyam <[email protected]> Co-authored-by: getzze <[email protected]> Co-authored-by: Lili Karashchuk <[email protected]> Co-authored-by: Sidharth Srinath <[email protected]> Co-authored-by: sidharth srinath <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: KevinZ0217 <[email protected]> Co-authored-by: Elizabeth <[email protected]> Co-authored-by: Talmo Pereira <[email protected]> Co-authored-by: eberrigan <[email protected]> Co-authored-by: vaibhavtrip29 <[email protected]> Co-authored-by: Keya Loding <[email protected]> Co-authored-by: Keya Loding <[email protected]> Co-authored-by: Hajin Park <[email protected]> Co-authored-by: Elise Davis <[email protected]> Co-authored-by: gqcpm <[email protected]> Co-authored-by: Andrew Park <[email protected]> Co-authored-by: roomrys <[email protected]> Co-authored-by: MweinbergUmass <[email protected]> Co-authored-by: Max Weinberg <[email protected]> Co-authored-by: DivyaSesh <[email protected]> Co-authored-by: Felipe Parodi <[email protected]> Co-authored-by: croblesMed <[email protected]>
Description
Previously, the
VideoTablesModeldisplayed the full path where the video is located with the name of the video. This resulted in unnecessarily long string as mentioned in Discussion #1855.This PR separates the filename into filepath and name of the video in the
VideosModelTableso that the users can comfortably see what file they are working on.Types of changes
Does this address any currently open issues?
#1855
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit