Shared axes colorbars & finer location control #956

Merged
merged 5 commits into from May 14, 2013

Conversation

Projects
None yet
6 participants
@pelson
Member

pelson commented Jun 19, 2012

I have implemented a particularly useful feature to be able to add a colorbar, via plt.colorbar, which can represent more than one axes. The way the code is implemented means that it also allows users to have more than one colorbar per axes (as one of the tests show).

shared axes colorbar

Additionally, I have added a location argument to the figure.colorbar function, the valid options are 'left', 'right', 'top', 'bottom', with (hopefully) obvious meaning. I have found it hard to write documentation on this particular feature because there is a duplication in the way that colorbar axes are created (use_gridspec), for which I have not implemented the same features.

shared axes colorbar

I have added a test_colorbar in the knowledge that #766 will also be adding a similar test file (although the content is completely separate).

@pelson

View changes

lib/matplotlib/colorbar.py
- ax.xaxis.set_label_position('bottom')
+ # location is either one of 'left' or 'right'
+ ax.xaxis.set_label_position(self.ticklocation)
+ # XXX This wasn't enabled before...

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Jun 20, 2012

Member

This will need removing. It was a flag to show that the command set_ticks_position wasn't necessary before.

@pelson

pelson Jun 20, 2012

Member

This will need removing. It was a flag to show that the command set_ticks_position wasn't necessary before.

@pelson

View changes

lib/matplotlib/colorbar.py
- anchor = kw.pop('anchor', (0.0, 0.5))
- panchor = kw.pop('panchor', (1.0, 0.5))
+@docstring.Substitution(make_axes_kw_doc)
+def make_axes(parents, location=None, orientation=None, fraction=0.15, shrink=1.0, aspect=20, **kw):

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Jun 20, 2012

Member

docstring needs adding back in.

@pelson

pelson Jun 20, 2012

Member

docstring needs adding back in.

@pelson

View changes

lib/matplotlib/testing/compare.py
@@ -209,6 +209,9 @@ def compare_images( expected, actual, tol, in_decorator=False ):
actual = convert(actual)
expected = convert(expected)
+ if not os.path.exists(expected):
+ raise IOError('Baseline image %r does not exist.' % expected)

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Jun 20, 2012

Member

This gets triggered when the baseline images don't yet exist. It might be better moving this to above the convert & adding a check on the actual image existing too.

@pelson

pelson Jun 20, 2012

Member

This gets triggered when the baseline images don't yet exist. It might be better moving this to above the convert & adding a check on the actual image existing too.

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Jun 20, 2012

Member

Is it just me, or are some of the tick marks for the colorbars are not snapping to the divisions in the colormap? For example, the "800" tick for the left-most colorbar in the first image of this PR.

Member

WeatherGod commented Jun 20, 2012

Is it just me, or are some of the tick marks for the colorbars are not snapping to the divisions in the colormap? For example, the "800" tick for the left-most colorbar in the first image of this PR.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Jun 20, 2012

Member

Is it just me...

No. However, I also get this on master.

Member

pelson commented Jun 20, 2012

Is it just me...

No. However, I also get this on master.

@jdh2358

This comment has been minimized.

Show comment Hide comment
@jdh2358

jdh2358 Jul 14, 2012

Collaborator

This looks like it needs rebasing. Also, the examples you include in the discussion are very nice, particularly the one with hatching. Could you provide some free-standing example/demo code for this example?

Collaborator

jdh2358 commented Jul 14, 2012

This looks like it needs rebasing. Also, the examples you include in the discussion are very nice, particularly the one with hatching. Could you provide some free-standing example/demo code for this example?

lib/matplotlib/figure.py
@@ -1227,16 +1227,16 @@ def savefig(self, *args, **kwargs):
ax.patch.set_edgecolor(cc[1])
@docstring.dedent_interpd
- def colorbar(self, mappable, cax=None, ax=None, **kw):
+ def colorbar(self, mappable, cax=None, ax=None, use_gridspec=False, **kw):

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Jul 24, 2012

Owner

I switched the use_gridspec default to True in #774; I don't know of any disadvantages. You might take this into account if/when you rebase.

@efiring

efiring Jul 24, 2012

Owner

I switched the use_gridspec default to True in #774; I don't know of any disadvantages. You might take this into account if/when you rebase.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 11, 2012

Owner

I have a branch in which this is merged into master, passing all tests; I think it makes sense for any additional changes to start from that point, but I am not sure what is the best way to facilitate this. A pull request against the original pelson branch does not seem to be doing the right thing.

Owner

efiring commented Aug 11, 2012

I have a branch in which this is merged into master, passing all tests; I think it makes sense for any additional changes to start from that point, but I am not sure what is the best way to facilitate this. A pull request against the original pelson branch does not seem to be doing the right thing.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 11, 2012

Member

@efiring: I will rebase this branch and do any outstanding actions from the PR in the week.
I have a couple of PRs which are in a similar state, all of which need just a little bit more effort before hopefully being merged in the next week or so! :-)

Member

pelson commented Aug 11, 2012

@efiring: I will rebase this branch and do any outstanding actions from the PR in the week.
I have a couple of PRs which are in a similar state, all of which need just a little bit more effort before hopefully being merged in the next week or so! :-)

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 13, 2012

Member

Replying to a comment in #774:

@pelson: are you going to be able to support use_gridspec in your improved colorbar positioning scheme, #956?

That's hard to answer. I haven't used gridspec thoroughly so I'm not entirely sure. If I'm honest, its not high up enough on my priorities to get a good look-in before the freeze (7 days) and I'm not too worried by the feature imparity for the time being (or by the fact that an extra keyword or two would be needed to trigger this code path). Eventually however, I will want to get the same location functionality into the make_axes_gridspec.

Member

pelson commented Aug 13, 2012

Replying to a comment in #774:

@pelson: are you going to be able to support use_gridspec in your improved colorbar positioning scheme, #956?

That's hard to answer. I haven't used gridspec thoroughly so I'm not entirely sure. If I'm honest, its not high up enough on my priorities to get a good look-in before the freeze (7 days) and I'm not too worried by the feature imparity for the time being (or by the fact that an extra keyword or two would be needed to trigger this code path). Eventually however, I will want to get the same location functionality into the make_axes_gridspec.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 19, 2012

Member

This PR is ready to go. I'm going to put the 1.2.x milestone on it, but if anyone in uncomfortable with that, I would sooner see some of my other PRs in the release.

Member

pelson commented Aug 19, 2012

This PR is ready to go. I'm going to put the 1.2.x milestone on it, but if anyone in uncomfortable with that, I would sooner see some of my other PRs in the release.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Aug 20, 2012

Owner

Let's see if we can resolve this against #774 before merging.

Owner

mdboom commented Aug 20, 2012

Let's see if we can resolve this against #774 before merging.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 21, 2012

Member

I'm taking this out of the 1.2.x milestone. We have many new features in the 1.2.x release, this needn't be one of them. #774 is likely to be more valuable at this stage. This will be a full fledged feature by the next release.

PR paused

Member

pelson commented Aug 21, 2012

I'm taking this out of the 1.2.x milestone. We have many new features in the 1.2.x release, this needn't be one of them. #774 is likely to be more valuable at this stage. This will be a full fledged feature by the next release.

PR paused

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Sep 28, 2012

Member

It looks like PR #774 was merged, so I think it's safe to continue with this pull request.

As far as I can see, it needs an example and an entry in whats_new.rst. There is also an outstanding clash with @efiring's diff in #774 where use_gridspec was changed to True.

Member

dmcdougall commented Sep 28, 2012

It looks like PR #774 was merged, so I think it's safe to continue with this pull request.

As far as I can see, it needs an example and an entry in whats_new.rst. There is also an outstanding clash with @efiring's diff in #774 where use_gridspec was changed to True.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Feb 19, 2013

Owner

@pelson, it looks like a lot of work went into this; I hope that rebasing and finishing it won't be too onerous.

Owner

efiring commented Feb 19, 2013

@pelson, it looks like a lot of work went into this; I hope that rebasing and finishing it won't be too onerous.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Apr 27, 2013

Owner

@pelson, can this make it for 1.3?

Owner

efiring commented Apr 27, 2013

@pelson, can this make it for 1.3?

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Apr 27, 2013

Member

@pelson, can this make it for 1.3?

I don't see why not. It will only work when not using gridspec (which admittedly is now default, right?), but it makes a lot of cases that I have to deal with easier when Gridspec isn't being used. The next step would be then to add similar support for Gridspec cases, I'm not sure whether I will be able to find time to do that before v1.3 though.

Thoughts?

Member

pelson commented Apr 27, 2013

@pelson, can this make it for 1.3?

I don't see why not. It will only work when not using gridspec (which admittedly is now default, right?), but it makes a lot of cases that I have to deal with easier when Gridspec isn't being used. The next step would be then to add similar support for Gridspec cases, I'm not sure whether I will be able to find time to do that before v1.3 though.

Thoughts?

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Apr 28, 2013

Owner

@pelson, I think you should go ahead, and worry about solving the gridspec problem later. The incompatibility can be mentioned in the docstrings.

Owner

efiring commented Apr 28, 2013

@pelson, I think you should go ahead, and worry about solving the gridspec problem later. The incompatibility can be mentioned in the docstrings.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Ok, I've rebased and fixed one of the tests which was failing due to the rebase. I think this is good to go...

Member

pelson commented May 14, 2013

Ok, I've rebased and fixed one of the tests which was failing due to the rebase. I think this is good to go...

lib/matplotlib/testing/compare.py
@@ -300,6 +300,9 @@ def compare_images( expected, actual, tol, in_decorator=False ):
actual = convert(actual, False)
expected = convert(expected, True)
+ if not os.path.exists(expected):
+ raise IOError('Baseline image %r does not exist.' % expected)
+

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

This should go above the if clause immediately above it, so we get the nicer error message for all file types.

@mdboom

mdboom May 14, 2013

Owner

This should go above the if clause immediately above it, so we get the nicer error message for all file types.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom May 14, 2013

Owner

This looks good other than my minor comment above.

Owner

mdboom commented May 14, 2013

This looks good other than my minor comment above.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson May 14, 2013

Member

Just did a rebase after a pyplot conflict with the xkcd change.

Member

pelson commented May 14, 2013

Just did a rebase after a pyplot conflict with the xkcd change.

efiring added a commit that referenced this pull request May 14, 2013

Merge pull request #956 from pelson/multiaxes_colorbar
Shared axes colorbars & finer location control

@efiring efiring merged commit d5b078b into matplotlib:master May 14, 2013

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring May 14, 2013

Owner

Although there are things still to be done--handling gridspec, and adding examples and documentation--it seems reasonable to go ahead and include this backwards-compatible incremental progress now, so you don't have to rebase it again and again.

Owner

efiring commented May 14, 2013

Although there are things still to be done--handling gridspec, and adding examples and documentation--it seems reasonable to go ahead and include this backwards-compatible incremental progress now, so you don't have to rebase it again and again.

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