-
Notifications
You must be signed in to change notification settings - Fork 588
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
Conversation
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.
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 thereSampleCollection.<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.
nit: PR conventions:
|
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.
LGTM
Resolves #1752
Example usage: