Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for customizing the output location of to_frames() #1798

Merged
merged 5 commits into from
Jul 3, 2022

Conversation

aturkelson
Copy link
Contributor

@aturkelson aturkelson commented May 27, 2022

Resolves #1752

Example usage:

import fiftyone as fo
import fiftyone.zoo as foz

# Default behavior is to sample into parallel image folders
dataset = foz.load_zoo_dataset("quickstart-video")
frames = dataset.to_frames(sample_frames=True)
fo.pprint(frames.shuffle()[:5].values("filepath"))

dataset.delete()

# New option: sample into a new location
dataset = foz.load_zoo_dataset("quickstart-video")
frames = dataset.to_frames(
    sample_frames=True,
    output_dir="/tmp/quickstart-video",
    rel_dir=foz.find_zoo_dataset("quickstart-video"),
)
fo.pprint(frames.shuffle()[:5].values("filepath"))

@aturkelson aturkelson requested a review from a team May 27, 2022 15:07
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever adding a new parameter to a view stage, there are a few additional places to add/document the parameter:

  • The corresponding SampleCollection.<view_stage>() method
  • The corresponding fiftyone.core.stages.<ViewStage> class
  • The view docs

Background
You'll note that the <ViewStage> class docstring and SampleCollection.<view_stage>() method docstrings are near copies of each other with slight example syntax changes. It's cumbersome/duplicative, but the motivation is:

  • <ViewStage> class is where stuff is implemented, so we document there
  • SampleCollection.<view_stage>() is the primary way a user will actually use the feature, so we want everything documented there so they can inspect the docstring to learn about all relevant options

Obviously view stages have many possible configurations. It's implementer's discretion whether a particular syntax, such as using to_frames(..., output_dir=), is "important" enough to warrant an explicit example in either the Examples:: section of the docstrings or a reference in the view docs.

This PR
Definitely document output_dir in the SampleCollection.to_frames() docstring, for the reasons mentioned above.

You'll notice that we (I) got lazy and used **kwargs in the implementation, which I generally don't do, but did do in the to_XXX() methods because they all have a relatively large number of optional parameters and the level of code duplication got annoying. I think I should have explicitly listed all parameters though. The current middle ground is silly, and I do feel its important for all parameters to be documented directly on to_frames() or else users won't discover as easily how to use them.

I was a different kind of lazy in ToFrames, where I used both **kwargs and then just pointed the reader to make_frames_dataset() in the docstring for parameter descriptions. That's pretty silly too... Probably should have copied kwargs and docs over there, but I didn't do it because I believe the ViewStage classes aren't directly used very often...

tldr: documentation and code duplication levels are pretty high when working on view stages. If you have any ideas to improve the situation from a dev perspective, feel free to share.

fiftyone/core/video.py Outdated Show resolved Hide resolved
fiftyone/core/video.py Outdated Show resolved Hide resolved
fiftyone/utils/video.py Outdated Show resolved Hide resolved
@brimoor
Copy link
Contributor

brimoor commented May 31, 2022

nit: PR conventions:

  • Add @voxel51/developers (or specific people, if appropriate) as reviewers
  • Add yourself (or whoever should merge the PR once it is approved) as assignee
  • Add appropriate labels to track type of work (for metrics, to guide reviewers on whether they are appropriate reviewers, etc)

@aturkelson aturkelson self-assigned this Jun 2, 2022
@brimoor brimoor changed the title [WIP] exposing new output_dir parameter to be used by to_frames Adding support for customizing the output location of to_frames() Jul 2, 2022
Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brimoor brimoor merged commit 8a2ef7c into develop Jul 3, 2022
@brimoor brimoor deleted the fr-1752-to-frames branch July 3, 2022 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Add destination directory for frame images generated by to_frames()
2 participants