-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
matplotlib2.0.0b4 compatibility fix (1/2) #867
Conversation
Requested change looks fine to me. I'll let @philippjfr decide whether it can be merged right away... |
@@ -15,6 +16,7 @@ | |||
from ..util import dynamic_update | |||
from .plot import MPLPlot | |||
from .util import wrap_formatter | |||
from distutils.version import LooseVersion |
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.
I'm not sure we should assume distutils is available (which might explain the current test failures).
axis.set_axis_bgcolor(self.bgcolor) | ||
|
||
if LooseVersion(mpl.__version__) <= '1.5.9': | ||
axis.set_axis_bgcolor(self.bgcolor) |
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.
To avoid relying on LooseVersion
, maybe we could check that 2
is the major version instead? i.e something like if mpl.__version__[0] == '2':
? That would also make it clear that the change is occurring in the matplotlib 2+.
Distutils is in the standard library no? We also use LooseVersion in several places already. The test failures are down to it trying to import PyQt4 for some reason. Something going wrong setting the agg backend? |
Looks like distutils is in the standard library so I have no idea why this PR would make the tests fail. |
Hmm, looks like it is currently a problem on master as well. |
Test failures are unrelated and this fix looks fine. Going to merge now and then fix the tests separately. |
in the mean time bryevdv suggests to do it a different way`(for bokeh similar fix), avoiding to use LooseVersion:
or maybe
... maybe next time |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
#866