-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
cropping-related fixes in analyze_videos and create_labeled_video
#3161
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
Draft
deruyter92
wants to merge
14
commits into
DeepLabCut:main
from
deruyter92:jaap/refactor_labeled_video_creation
Draft
cropping-related fixes in analyze_videos and create_labeled_video
#3161
deruyter92
wants to merge
14
commits into
DeepLabCut:main
from
deruyter92:jaap/refactor_labeled_video_creation
+81
−32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Faulty conditional was evaluating whether the cropping parameter in config equals string "True", rather than boolean True. This lead to unintended false evaluation for cropping field with boolean value True.
In the original behaviour, cropping=None was interpreted as read cropping params from config. This is in contrast to most other occurrences (`video_inference`, `gui.tabs.analyze_videos`, `VideoIterator`), which interpret cropping=None as no cropping. This leads to potential conflicts. This commit changes the behaviour for cropping=None, which is now interpreted as no cropping (like elsewhere). For backwards compatibility, the default argument is changed to 'from_config' which follows prior behavior.
Bounding box positions were corrected by the crop margin, even when the output video is also cropped (which should not be the case). This leads to misplaced bounding boxes. This commit fixes the unintended behaviour where bounding box positions are always corrected: i.e. bounding boxes are only corrected when cropping was used during video AND the output video is not cropped.
In prior implementation, whether to use cropping or not was determined by the cropping coordinates, rather than the boolean cropping flag in video metadata. When metadata specifies cropping=False, now no cropping is applied.
2 tasks
Contributor
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.
Pull request overview
This PR addresses several functional issues in the cropping functionality for video analysis and labeled video creation in DeepLabCut. The changes improve consistency in how cropping parameters are handled across TensorFlow and PyTorch implementations, fix a bug in GUI configuration evaluation, and correct bounding box coordinate adjustments in CreateVideo.
Key changes:
- Changed the default cropping behavior to use
'from_config'instead ofNone, with explicit handling for when users want no cropping - Fixed GUI config evaluation to properly interpret boolean values instead of string comparisons
- Corrected bounding box coordinate offset logic to only apply when generating full videos from cropped coordinates
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| deeplabcut/utils/make_labeled_video.py | Added conditional logic to only apply crop margins to bbox coordinates when cropping is enabled and displaying uncropped video; added cropping parameter to create_video function |
| deeplabcut/pose_estimation_tensorflow/predict_videos.py | Updated analyze_videos to support 'from_config' as default cropping value with explicit None handling; updated docstring |
| deeplabcut/pose_estimation_pytorch/apis/videos.py | Updated analyze_videos to support 'from_config' as default cropping value; added warning when config has cropping but None is passed; updated docstrings and type hints |
| deeplabcut/gui/tabs/analyze_videos.py | Fixed config evaluation to use boolean check instead of string comparison "True"; changed tuple to list for consistency |
| deeplabcut/compat.py | Updated type hints and docstring for cropping parameter in analyze_videos wrapper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
...opping='from_config'
...re cropped in `extract_outlier_frames`. This commit fixes a bug that introduces a mismatch between label-coordinates and video, when extracting cropped outlier frames. When the `analyze_videos` step was performed using a cropping window. The predicted keypoints are stored in the crop-window space. In the previous implementation, the keypoints are ALWAYS converted back to reflect full-video coordinates by adding the left and top crop margins (introduced in PR DeepLabCut#2538). However the keypoints should only be converted back if the output frames are full-video, not when they are cropped. This commit fixes that by using the difference between the output margin and the margin stored in metadata.
...refactor_labeled_video_creation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR addresses some functional vulnerabilities in the code for creating labeled videos. Potentially related to #3130, #3060 and #1979.
The following changes are implemented:
Fix disfunctional evaluation of config in gui.tabs.analyze_videos. In the previous code, the configuration value for
croppingis misinterpreted because the conditional compares to a string value instead of a boolean. This is now changed to boolean evaluation.Ambivalous interpretation of argument
cropping=None.In most places,
Noneis passed to reflect that no cropping should be applied, but in pytorch version ofanalyze_videos,Noneresulted in cropping coordinates loaded from config.yaml. This PR changes the behaviour as follows: The default argument is changed tocropping='from_config'which follows prior behavior;cropping=Noneis now interpreted as no cropping to align with its interpretation elsewhere,cropping=[x1, x2, y1, y2]is still interpreted as cropping with the specified parameters.Crop margins are always added to the bounding box coordinates when plotting in CreateVideo, even when the output is a cropped video and the coordinates therefore already reflected the correct values. This is a bug that is now fixed in this PR. Margins should only be added when using coordinates from a cropped video to generate the full video.
When metadata specifies cropping=False, this is still interpreted as cropping=True when the crop parameters don't match the video dimensions. Although this should normally cause no problems, it would be better to always follow the the value specified in metadata. This is now corrected in this PR.