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

Conversation

Projects
None yet
3 participants
@stonebig
Contributor

stonebig commented Sep 18, 2016

@jlstevens

This comment has been minimized.

Member

jlstevens commented Sep 18, 2016

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

This comment has been minimized.

@jlstevens

jlstevens Sep 18, 2016

Member

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)

This comment has been minimized.

@jlstevens

jlstevens Sep 18, 2016

Member

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

This comment has been minimized.

Member

philippjfr commented Sep 18, 2016

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

This comment has been minimized.

Member

jlstevens commented Sep 18, 2016

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

@jlstevens

This comment has been minimized.

Member

jlstevens commented Sep 18, 2016

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

@philippjfr

This comment has been minimized.

Member

philippjfr commented Sep 19, 2016

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 ioam:master Sep 19, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
s3-reference-data-cache Tests still building.
Details
@stonebig

This comment has been minimized.

Contributor

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