-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Shared axes colorbars & finer location control #956
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
Conversation
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... |
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.
This will need removing. It was a flag to show that the command set_ticks_position
wasn't necessary before.
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. |
No. However, I also get this on master. |
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? |
@@ -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): |
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 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.
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. |
@efiring: I will rebase this branch and do any outstanding actions from the PR in the week. |
Replying to a comment in #774:
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 |
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. |
Let's see if we can resolve this against #774 before merging. |
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 |
@pelson, it looks like a lot of work went into this; I hope that rebasing and finishing it won't be too onerous. |
@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? |
@pelson, I think you should go ahead, and worry about solving the gridspec problem later. The incompatibility can be mentioned in the docstrings. |
Ok, I've rebased and fixed one of the tests which was failing due to the rebase. I think this is good to go... |
@@ -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) | |||
|
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.
This should go above the if
clause immediately above it, so we get the nicer error message for all file types.
This looks good other than my minor comment above. |
Just did a rebase after a pyplot conflict with the xkcd change. |
Shared axes colorbars & finer location control
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. |
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).
Additionally, I have added a
location
argument to thefigure.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.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).