Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Reset the available animation movie writer on rcParam change #5628
Conversation
tacaswell
added the
needs_review
label
Dec 6, 2015
tacaswell
added this to the
proposed next point release (2.1)
milestone
Dec 8, 2015
janschulz
commented on the diff
Dec 9, 2015
| if writerClass.isAvailable(): | ||
| self.avail[name] = writerClass | ||
| return writerClass | ||
| return wrapper | ||
| + def endsure_not_dirty(self): | ||
| + """If dirty, reasks the writers if they are available""" | ||
| + if self._dirty: | ||
| + self.reset_available_writers() | ||
| + | ||
| + def reset_available_writers(self): | ||
| + """Reset the available state of all registered writers""" | ||
| + self.avail = {} | ||
| + for name, writerClass in self._registered.items(): | ||
| + if writerClass.isAvailable(): |
janschulz
Contributor
|
|
Apart from the possible deprecation (which should go to a different PR), IMO this is ready to go in... |
dopplershift
and 1 other
commented on an outdated diff
Dec 28, 2015
|
Ping? |
dopplershift
and 1 other
commented on an outdated diff
Feb 6, 2016
| @@ -802,6 +802,21 @@ def validate_hist_bins(s): | ||
| raise ValueError("'hist.bins' must be 'auto', an int or " + | ||
| "a sequence of floats") | ||
| +def validate_animation_writer_path(p): | ||
| + # Make sure it's a string and then figure out if the animations | ||
| + # are already loaded and reset the writers (which will validate | ||
| + # the path on next call) | ||
| + if not isinstance(p, six.text_type): | ||
| + raise ValueError("path must be a (unicode) string") | ||
| + from sys import modules | ||
| + # set dirty, so that the next call to the reistry will reevaluate |
|
|
|
Minor comment nit, otherwise this is |
janschulz
added some commits
Dec 6, 2015
|
cleaned up the typo and repushed |
WeatherGod
commented on an outdated diff
Feb 6, 2016
| @@ -163,6 +166,30 @@ def animate(i): | ||
| frames=iter(range(5))) | ||
| +def test_movie_writer_registry(): | ||
| + ffmpeg_path = mpl.rcParams['animation.ffmpeg_path'] | ||
| + # Not sure about the first state as there could be some writer | ||
| + # which set rcparams | ||
| + #assert_false(animation.writers._dirty) | ||
| + assert_true(len(animation.writers._registered) > 0) | ||
| + animation.writers.list() # resets dirty state | ||
| + assert_false(animation.writers._dirty) | ||
| + mpl.rcParams['animation.ffmpeg_path'] = u"not_available_ever_xxxx" | ||
| + assert_true(animation.writers._dirty) | ||
| + animation.writers.list() # resets | ||
| + assert_false(animation.writers._dirty) | ||
| + assert_false(animation.writers.is_available("ffmpeg")) | ||
| + # something which is garanteered to be available in path |
|
|
|
Appveyor is having all sorts of failures, some of them having to do with animation stuff, so I am not entirely certain if it is the usual failures or not. |
|
The imagemagic problem is that the workaround for masking It seems that the workaround should also be applied in the And for your daily dose of WTF: and running the tests with |
|
I also currently have a bug that mencoder returns 3: I've actually no clue why that's happens :-/ Anyone with mencoder knowledge here? |
|
The error seems to be this (pull from |
|
It seems the problem is this: E.g. here: http://ehc.ac/p/mplayer-win32/bugs/179/ -> unanswered :-( I use this build: http://mplayerwin.sourceforge.net/downloads.html (build mplayer-svn-37610-x86_64.7z) -> not a mpl bug and so IMO if this passes (without mplayer/mencoder, which is not installed on appveyor) and no one finds another typo :-), this is IMO good to merge... |
janschulz
and 1 other
commented on an outdated diff
Feb 7, 2016
| @@ -432,9 +456,16 @@ def finish(self): | ||
| # Check error code for creating file here, since we just run | ||
| # the process here, rather than having an open pipe. | ||
| if self._proc.returncode: | ||
| + try: | ||
| + stdout = [s.decode() for s in self._proc._stdout_buff] | ||
| + stderr = [s.decode() for s in self._proc._stderr_buff] | ||
| + verbose.report("MovieWriter.finish: stdout: %s" % stdout, level='helpful') | ||
| + verbose.report("MovieWriter.finish: stderr: %s" % stderr, level='helpful') | ||
| + except Exception as e: | ||
| + pass |
janschulz
Contributor
|
tacaswell
and 1 other
commented on an outdated diff
Feb 7, 2016
| ImageMagickBase._init_from_registry() | ||
| @writers.register('imagemagick') | ||
| -class ImageMagickWriter(MovieWriter, ImageMagickBase): | ||
| +class ImageMagickWriter(ImageMagickBase, MovieWriter, ): |
janschulz
Contributor
|
|
Looks like some long lines have crept in |
|
|
janschulz
added some commits
Feb 7, 2016
|
Oh damn, I really should add a pep8 precommit / prepush hook in the matplotlib repo... Ok, added comments and hopefully fixed the PEP8 thingies... |
tacaswell
added a commit
that referenced
this pull request
Feb 7, 2016
|
|
tacaswell |
424556b
|
tacaswell
merged commit 424556b
into matplotlib:master
Feb 7, 2016
mdboom
removed the
needs_review
label
Feb 7, 2016
|
@tacaswell, it looks to me like this could be backported to v2.x, in the spirit of making 2.0 less likely to trip people up. |
|
I completely forget about this and was contemplating re-writing it to fix issues that Christoph reported. |
tacaswell
added a commit
to tacaswell/matplotlib
that referenced
this pull request
Oct 19, 2016
|
|
tacaswell |
70418b9
|
NelleV
added a commit
that referenced
this pull request
Oct 19, 2016
|
|
NelleV |
f91dff4
|
|
backported to v2.x via #7306 |
janschulz commentedDec 6, 2015
If one of the rcParams for a path to a program, which was called by a
movie writer, is changed, the the available movie writer in the
registry should be reevaluated if they are (still/became) available.
This also fixes the problem that you have to set the path to a movie
writer before importing mpl.animation, as before the state was
fixed on import time.
Closes: #5616