Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Extract clip as video, slp and pkg.slp #2059
base: develop
Are you sure you want to change the base?
Extract clip as video, slp and pkg.slp #2059
Changes from 24 commits
c3a8e5b
dcfa897
39c17b5
945d737
1907daf
c31caa8
9f71476
117d5a3
351919e
fd1fc67
08b40cf
ab58597
5f6fb07
4c4ea5d
c964f2a
ace2c85
ddcb213
2dccc28
4bc5422
2d00bb8
f5c617f
e972d3f
39cc496
b529dd8
cd866f3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider creating a base class for clip export commands.
There's significant code duplication between
ExportClipVideo
andExportClipPkg
, particularly in frame range validation and labels extraction logic. Consider creating a base class to share common functionality.Also applies to: 3645-3646
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we say
and raise the no valid clip range error below if
frame_range is None
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
frame_range == (0, video.frames)
not a valid choice?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I thought a "clip" meant it wasn't the whole video, but that's just semantics. I went ahead and made that a valid choice.
Unfortunately this has lead to another discovery which has lead me to open a new issue. When I do select the maximum frame range from (0, video.frames) the GUI the selection and frame count display does not match the video. I have opened a new issue at #2074 to describe the mismatch between the video frames and the Shift + Click frame selection tool on the Timeline viewer.
When I select the full clip range I get the ValueError: Frame range (0, 1505) is outside video bounds [0, 1500]. So this PR depends on Issue #2074 being fixed before we can move forward with this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we want to run into this case in
Labels.extract
->Labels.__getitem__
->Labels.get
:sleap/sleap/io/dataset.py
Lines 763 to 764 in 66d96ce
which will call this case of
Labels.find
->Labels._cache.find_frames
:sleap/sleap/io/dataset.py
Lines 142 to 147 in 66d96ce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Revise labels extraction implementation.
The current implementation might not handle frame indices correctly. Consider using the suggested approach from the codebase maintainer.
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling with proper exception chaining.
Use exception chaining to preserve the original error context.
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.8.2)
3549-3549: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use pathlib for path operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this
do_action
is the same as theExportClipVideo
class... do you think there is a way we can maybe break out the subparts ofdo_action
into class methods and either combine theExportClipVideo
andExportClipPkg
classes into one class OR create create a base class and subclass to re-use code?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling in package save operation.
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.8.2)
3661-3661: Local variable
e
is assigned to but never usedRemove assignment to unused variable
e
(F841)
3662-3662: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)