Skip to content

Parallel movie writing routines. #4509

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

Closed
wants to merge 4 commits into from
Closed

Parallel movie writing routines. #4509

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 8, 2015

Parallel movie recording routines for Python's matplotlib (see #2820 for the motivation and the discussion on design/implementation).

William Yessen added 2 commits June 8, 2015 14:16
Parallel movie recording routines for Python's matplotlib (see #2820 for the motivation and the discussion on design/implementation).
Example usage for mpl_parsave
# processes.
# ========================================================================#
class Parsave:
def __init__(self):
Copy link
Member

Choose a reason for hiding this comment

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

4 spaces please to match the rest of the library.

@tacaswell tacaswell added this to the proposed next point release milestone Jun 8, 2015
@tacaswell
Copy link
Member

attn @WeatherGod @dopplershift

Changed tabs from two to four spaces.
@ghost
Copy link
Author

ghost commented Jun 9, 2015

Hmm... can't see why the build failed.

@tacaswell
Copy link
Member

There are intermittent failures related to latex, restarted it.

@ivanov
Copy link
Member

ivanov commented Jun 9, 2015

Thank you for your contribution, @william-yessen, this seems like it'll be a nice feature to add to matplotlib. I happened to browse github casually today, so I don't have detailed feedback on the implementation, my review is mostly style related. If you are able, I encourage you to take some time to meet our coding style guide. In particular:

  1. many of the comments you left should be changed to docstrings, so the information is readily available when using tools like IPython.
  2. classes should inherit from object (and start with a capital, as per PEP8)
  3. I'd use the basic animation example - since you're trying to show off the saving capabilities, but either way, could you refer to the example you choose more explicity (# THIS CODE COMES FROM animation/basic_example.py)
  4. add some mention of this new toolkit in new file in doc/mpl_toolkits/index.rst, and also make an entry for it in a new file doc/users/whats_new directory (see the README.rst that's there). That way folks can find out about it once it's in place.

Let us know if you have any questions (or if it sounds like too much work).

@ghost
Copy link
Author

ghost commented Jun 12, 2015

@ivanov

Thanks for the tips! No, it's not a problem, I can do all of that. Been a bit busy these days, maybe next week.

@ghost
Copy link
Author

ghost commented Nov 14, 2015

Hmm... any idea why the checks failed?

@tacaswell
Copy link
Member

There are intermittent failures in the test suite.

Closed/re-opened PR to re-trigger the test suite on top of current master.

@ghost
Copy link
Author

ghost commented Nov 14, 2015

Thanks, Thomas!

@mdboom
Copy link
Member

mdboom commented Nov 17, 2015

Do we want to merge this as-is, or does this make sense as a separate, but affiliated, project?

@ghost
Copy link
Author

ghost commented Nov 17, 2015

@mdboom Currently it resides in the toolkits. Would that be an appropriate place for it?

@tacaswell
Copy link
Member

The example should move into examples so that it ends up on the website and so we don't ship it in the tarball.


class Parsave(object):
"""
Parsave: class containing static methods to fascilitate parallel recording.
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the code-base is 4 space indent

@ghost
Copy link
Author

ghost commented Nov 17, 2015

OK, all of the comments make sense. I'll work on it a bit later.

@tacaswell
Copy link
Member

This should also get an entry in https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new

Sorry to bombard you with mostly trivial comments, but keeping the style across the code base consistent does make maintenance easier in the long run.

@WeatherGod
Copy link
Member

How would this work together with #5454?

On Tue, Nov 17, 2015 at 2:13 PM, Thomas A Caswell notifications@github.com
wrote:

This should also get an entry in
https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new

Sorry to bombard you with mostly trivial comments, but keeping the style
across the code base consistent does make maintenance easier in the long
run.


Reply to this email directly or view it on GitHub
#4509 (comment)
.

@mdboom
Copy link
Member

mdboom commented Nov 17, 2015

@mdboom Currently it resides in the toolkits. Would that be an appropriate place for it?

It's not inappropriate. But we are trying to move away from "incorporating" new toolkits and just releasing tools as separate projects, on their own release schedule etc.

I don't have strong opinions about this one -- it's pretty small and self-contained. I could be persuaded either way.

@tacaswell
Copy link
Member

Is there a way to turn this into something that looks like the 'normal' writer object?

@ghost
Copy link
Author

ghost commented Nov 18, 2015

@tacaswell Yes, I think so. I toyed with the idea a while back, but figured at the end that the way it is currently implemented is simpler and easier to maintain. If, however, there is sufficient reason for it to be a 'normal' writer object, I can work on that.

@tacaswell
Copy link
Member

The argument in favor of making it all the same api is that now users can just change a class to flip between parallel vs not moving writing which makes their code simpler and easier to maintain.

@tacaswell
Copy link
Member

Closing because of lack of comments for almost 2 years and the OP has deleted their account.

@tacaswell tacaswell closed this Aug 29, 2017
@QuLogic QuLogic modified the milestones: unassigned, 2.1 (next point release) Aug 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants