Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Reset the available animation movie writer on rcParam change#5628
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| """Reset the available state of all registered writers""" | ||
| self.avail = {} | ||
| for name, writerClass in self._registered.items(): | ||
| if writerClass.isAvailable(): |
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.
BTW: should that be "is_available"? It's the only method in that class(es) which use CamelCase instead of underscores...
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.
It is annoying, but I don't think worth breaking user code over.
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.
At most, we can rename and add an aliasisAvailable ->is_available with the@deprecated decorator.
7d847ea tob46d8aaCompareApart from the possible deprecation (which should go to a different PR), IMO this is ready to go in... |
| if writerClass.isAvailable(): | ||
| self.avail[name] = writerClass | ||
| return writerClass | ||
| return wrapper | ||
| def endsure_not_dirty(self): |
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.
shouldendsure beensure?
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.
Omg, fix is on the way...
Ping? |
| 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 |
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.
...registry will re-evaluate
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.
done and repushed
Minor comment nit, otherwise this is 👍 from me. |
If one of the rcParams for a path to a program, which was called by amovie writer, is changed, the the available movie writer in theregistry should be reevaluated if they are (still/became) available.This also fixes the problem that you have to set the path to a moviewriter before importing mpl.animation, as before the state wasfixed on import time.
python is blocking, leading to timeouts...
cleaned up the typo and repushed |
| 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 |
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.
"garanteered"?
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. |
On windows the check ensures that the imagemagick path for 'convert' wasset to '' if it was just 'convert' to prevent clashes with the windowsbuilt-in command 'convert' (doing filesystem conversion). Unfortunately,the command only set this values in the current rcParams and rcParamsDefault,but a use of `matplotlib.style.use("classic")` after importing mpl.animationwould again overwrite it with `convert`.In this case, the image comparison decorator sets the style to classic andappveyor went BOOM...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... |
This pulls out the stderr and stdout from the internal POpen buffers (testedon py3.5 and not shown if any error occures while pulling out the messages...).It also changes the exception message to mention mpl.verbose.set_level("helpful")because adding a commandline switch is probably not helpful in todays world whereeverybody uses the Jupyter notebook and can't add to the kernel commandline...It would be nice if this verbose level could be set by nose / tests.py, but Ihave found no way yet :-(| verbose.report("MovieWriter.finish: stdout: %s" % stdout, level='helpful') | ||
| verbose.report("MovieWriter.finish: stderr: %s" % stderr, level='helpful') | ||
| except Exception as e: | ||
| pass |
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.
This is not my finest code, but it works on py35...
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.
This is just to provide improved debugging feed back right?
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.
yes, when the called converter fails and the user adds thempl.verbose.set_level("helpful") before calling the failing method again...
Unfortunately, it doesn't work for the unittests unless I add such aset_level call in the unittest file :-(
Looks like some long lines have crept in |
👍 modulo small comments / pep8 |
Oh damn, I really should add a pep8 precommit / prepush hook in the matplotlib repo... Ok, added comments and hopefully fixed the PEP8 thingies... |
Reset the available animation movie writer on rcParam change
@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. |
Reset the available animation movie writer on rcParam change
Merge pull request#5628 from JanSchulz/ani_writer
backported to v2.x via#7306 |
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