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

WIP: adapt Brain.save_movie() API for imageio #155

Merged
merged 3 commits into from
Jun 30, 2016

Conversation

christianbrodbeck
Copy link
Collaborator

Following up on #154

imageio parameters: http://imageio.readthedocs.io/en/latest/format_ffmpeg.html#parameters-for-saving

Probably the most sense would make to provide access to all of imageio's options. I've saved a couple of movies and imageio seems to have sensible defaults (variable bitrate, hence I removed our fixed bitrate default). We could keep the existing keyword arguments so we don't break code that used positional arguments. Are there better alternatives? I am not sure how to document in case we just include a **kwargs in the signature. Should the documentation just include a link to imageio's documentation?

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage decreased (-0.03%) to 76.162% when pulling 2a4e85d on christianbrodbeck:imageio into 89056ab on nipy:master.

Codec to use with ffmpeg (default 'mpeg4').
bitrate : str | float
Bitrate to use to encode movie. Can be specified as number (e.g.
64000) or string (e.g. '64k'). Default value is 1M
Copy link
Contributor

Choose a reason for hiding this comment

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

add kwargs to the docstring

@christianbrodbeck
Copy link
Collaborator Author

Updated. Keeping the specific keywords open would have the big advantage that additional imageio formats would become available, such as *.gif

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage decreased (-0.03%) to 76.162% when pulling fb5c56d on christianbrodbeck:imageio into 89056ab on nipy:master.

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage decreased (-0.03%) to 76.162% when pulling fb5c56d on christianbrodbeck:imageio into 89056ab on nipy:master.

bitrate : str | float
Bitrate to use to encode movie. Can be specified as number (e.g.
64000) or string (e.g. '64k'). Default value is 1M
additional keywords :
Copy link
Contributor

Choose a reason for hiding this comment

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

why additional keywords and not **kwargs :

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fine with me :)

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage decreased (-0.03%) to 76.162% when pulling 756a9a6 on christianbrodbeck:imageio into 89056ab on nipy:master.

@agramfort
Copy link
Contributor

ready to merge?

@christianbrodbeck
Copy link
Collaborator Author

If you're happy with the documentation

@agramfort agramfort merged commit 79beb15 into nipy:master Jun 30, 2016
@agramfort
Copy link
Contributor

ok

thx @christianbrodbeck

next time set the PR to MRG so I don't have to ask :)

@christianbrodbeck
Copy link
Collaborator Author

Ok, sorry :)

@christianbrodbeck christianbrodbeck deleted the imageio branch June 30, 2016 21:24
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.

3 participants