Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Deprecated the set_colorbar method on a scalar mappable. #2055

Merged
merged 2 commits into from May 24, 2013

Conversation

Projects
None yet
5 participants
Member

pelson commented May 23, 2013

No description provided.

@WeatherGod WeatherGod commented on the diff May 23, 2013

lib/matplotlib/cm.py
def set_colorbar(self, im, ax):
- 'set the colorbar image and axes associated with mappable'
- self.colorbar = im, ax
+ """set the colorbar and axes instances associated with mappable"""
+ self.colorbar = im
@WeatherGod

WeatherGod May 23, 2013

Member

I must have missed some other PR... there weren't any old code that would have queried and expected a tuple from the colorbar attribute, right?

@pelson

pelson May 24, 2013

Member

No. I don't think anything is making use of it (at least, there are no tests which exercise it, and a thorough grepping didn't throw anything up)

Owner

efiring commented May 23, 2013

It looks like part of the point here is that a colorbar always has an axes attribute, so there is no need for a tuple in the ScalarMappable colorbar attribute.

Looks like a good cleanup--but it is a bit dangerous, and I don't know how to handle that. The danger, of course, is that some user code is expecting the attribute to hold a tuple. Maybe it will be adequate to simply flag this prominently in the description of API changes.

Member

pelson commented May 24, 2013

Looks like a good cleanup--but it is a bit dangerous, and I don't know how to handle that. The danger, of course, is that some user code is expecting the attribute to hold a tuple. Maybe it will be adequate to simply flag this prominently in the description of API changes.

Agreed. Indeed I have code which does cb = scalarmappable.colorbar[0]. But I'm prepared to accept the fact that this is a backwards incompatible change - having anything but a colorbar instance in a colorbar attribute is just wrong. An api_changes.rst entry is the best I can come up with too - but I think this attribute is sufficiently opaque that you would only know about it if you read the source code of cm.ScalarMappable.

Member

pelson commented May 24, 2013

Think this is good to go.

mdboom added a commit that referenced this pull request May 24, 2013

Merge pull request #2055 from pelson/mappable_colorbar_attribute
Deprecated the set_colorbar method on a scalar mappable.

@mdboom mdboom merged commit 148ed82 into matplotlib:master May 24, 2013

1 check failed

default The Travis CI build failed
Details

@pelson pelson commented on the diff May 24, 2013

doc/api/api_changes.rst
@@ -117,6 +117,14 @@ Changes in 1.3.x
position type. Previously, it would draw the right or top spine at
+1 data offset.
+* The ScalarMappable class' set_colorbar is now deprecated. Instead, the
+ :attr:`matplotlib.cm.ScalarMappable.colorbar` attribute should be used.
+ In previous matplotlib versions this attribute was an undocumented tuple
+ of ``(colorbar_instance, colorbar_axes)`` but is now just
+ ``colorbar_instance``. To get the colorbar axes it is possible to just use
+ the :attr:`~matplotlib.colorbar.ColorbarBase.ax` attribute on a colorbar
+ isntance.
@pelson

pelson May 24, 2013

Member

Ah. Sorry - typo.

@mdboom

mdboom May 24, 2013

Owner

No worries. We can just push that fix directly to master.

Member

dmcdougall commented Jun 23, 2013

As a note, since this is a backwards incompatible change, should the next version number be 2.0.x?

Owner

mdboom commented Jun 24, 2013

Since 1.0, we have allowed backward incompatible changes of this magnitude for minor point releases, with a note in api_changes.rst and a deprecation process when necessary. So I don't think this requires 2.0.x. I think we should save that for the time when a significant portion of user code would require changes -- this is a bit of a corner case.

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 10, 2015

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 11, 2015

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment