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

matplotlib2.0.0b4 compatibility fix (1/2) #867

Merged
merged 2 commits into from
Sep 19, 2016
Merged

matplotlib2.0.0b4 compatibility fix (1/2) #867

merged 2 commits into from
Sep 19, 2016

Conversation

stonebig
Copy link
Contributor

@jlstevens
Copy link
Contributor

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
Copy link
Contributor

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)
Copy link
Contributor

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+.

@philippjfr
Copy link
Member

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?

@jlstevens
Copy link
Contributor

Looks like distutils is in the standard library so I have no idea why this PR would make the tests fail.

@jlstevens
Copy link
Contributor

jlstevens commented Sep 18, 2016

Hmm, looks like it is currently a problem on master as well.

@philippjfr
Copy link
Member

Test failures are unrelated and this fix looks fine. Going to merge now and then fix the tests separately.

@philippjfr philippjfr merged commit 2ff9edd into holoviz:master Sep 19, 2016
@stonebig
Copy link
Contributor Author

stonebig commented Sep 19, 2016

in the mean time bryevdv suggests to do it a different way`(for bokeh similar fix), avoiding to use LooseVersion:

if "get_facecolor" in dir(ax):

or maybe

if hasattr(ax, 'get_facecolor')

... maybe next time

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