youtube-vis: video dataset #486
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af08e50151
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/review |
michal-lightly
left a comment
michal-lightly
left a comment
There was a problem hiding this comment.
Thanks! I think we can simplify this PR.
I have not looked at the tests yet.
lightly_studio/src/lightly_studio/examples/example_video_annotations.py
Outdated
Show resolved
Hide resolved
| - Enabled editing of segmentation masks and deletion of annotations in the details view. | ||
| - Allowed users to customize toolbar shortcuts. | ||
| - Added `Sample.add_annotation()` method, adds annotations to samples. | ||
| - Loading videos with annotations from youtube-vis format via `dataset.add_videos_from_youtube_vis`. |
There was a problem hiding this comment.
We will also need to add info to docs, I assume that will be in a separate PR?
We will need to highlight that this is not the standard youtubevis format.
| allowed_extensions: An iterable container of allowed video file | ||
| extensions in lowercase, including the leading dot. If None, | ||
| uses default VIDEO_EXTENSIONS. |
There was a problem hiding this comment.
Please add information about how this is used - this is user-facing interface.
Naively, annotations.json should have all the info about the loaded videos. But there is a reason we need to supply path and allowed_extensions.
What is not clear to me is what happens if the info from annotations.json and the list of video files mismatches.
| def add_videos_from_youtube_vis( | ||
| self, | ||
| annotations_json: PathLike, | ||
| path: PathLike, |
There was a problem hiding this comment.
Optional: We could call it videos_path.
| ) | ||
|
|
||
|
|
||
| def _collect_video_file_paths( |
There was a problem hiding this comment.
Optional: Consider moving this to add_videos.py too.
|
|
||
| def test_load_video_annotations_from_labelformat( | ||
| db_session: Session, | ||
| patch_collection: None, # noqa: ARG001 |
There was a problem hiding this comment.
Why has this file changed to depend on the VideoDataset class? It would be better to keep it independent if possible, then we also don't need the patch.
| session: The database session. | ||
| dataset_id: The ID of the video dataset to load annotations into. | ||
| video_paths: An iterable of file paths to the videos to load. | ||
| input_labels: The labelformat input containing video annotations. |
There was a problem hiding this comment.
Also here, please add info on how the parameters interact (see the comment in VideoDataset).
| file_path = Path(video_file) | ||
| video_name_to_path[file_path.name] = str(file_path.absolute()) |
There was a problem hiding this comment.
Will this work also with fsspec paths?
| for video_file in video_paths: | ||
| file_path = Path(video_file) | ||
| video_name_to_path[file_path.name] = str(file_path.absolute()) | ||
| if file_path.stem in video_stem_to_path: |
There was a problem hiding this comment.
There is a problem with .stem(), it will strip any path to the video. I'd suggest .with_suffix("").
This assumes labelformat might return the video paths with a folder prefix, which I think is the case. Can we add a test with videos in some nested folder structure (e.g. a/vid.mp4, b/vid.mp4)? Can be a follow-up.
| if resolved_path is None: | ||
| raise FileNotFoundError(f"No video file found for '{filename}'.") | ||
| video_paths.append(resolved_path) | ||
| return list(dict.fromkeys(video_paths)) |
There was a problem hiding this comment.
| return list(dict.fromkeys(video_paths)) | |
| return video_paths |
| "length": 3, | ||
| }, | ||
| ], | ||
| "annotations": [ |
There was a problem hiding this comment.
Optional: We can keep this empty since it is irrelevant for the test.
| embed=True, | ||
| ) | ||
|
|
||
| # Verify embeddings were created |
There was a problem hiding this comment.
There is an easier way, from a test with images:
assert len(image1.sample.embeddings) == 1
2 participants
What has changed and why?
Adding the loading method for youtube-vis format to the video dataset.
How has it been tested?
tests are implemented.
Did you update CHANGELOG.md?