FIX/ENH CL: Allow single parent colorbar w/ gridspec layout #10629

jklymak · 2018-02-28T17:07:20Z

PR Summary

In the master version of constrained_layout (CL), a colorbar associated with a single parent axes (i.e. fig.colorbar(im, ax=axs[2])) is associated with the subplotspec layout box. If there is more than one parent, it is associated with the gridspec layout box that contains the subplotspecs (i.e. fig.colorbar(im, ax=axs[:2])).

As noted in #10627 by @afvincent this leads to a surprising result for a layout like:

import matplotlib.pyplot as plt
import numpy as np

def example_pcolor(ax, cmap='RdBu_r'):
    dx, dy = 0.6, 0.6
    y, x = np.mgrid[slice(-3, 3 + dy, dy),
                    slice(-3, 3 + dx, dx)]
    z = (1 - x / 2. + x ** 5 + y ** 3) * np.exp(-x ** 2 - y ** 2)
    pcm = ax.pcolormesh(x, y, z, cmap=cmap, vmin=-1., vmax=1.,
                        rasterized=True)
    return pcm

fig, axs = plt.subplots(3, 1, figsize=(4, 4), constrained_layout=True)
for ax in axs[:2]:
    pc = example_pcolor(ax)
pc2 = example_pcolor(axs[2], cmap='viridis')

fig.colorbar(pc, ax=axs[:2])
fig.colorbar(pc, ax=axs[2])

plt.show()

yields:

This is technically correct. We want to be able to have a single colorbar follow a single axes:

However, for this case it is undesirable. This PR changes the argument of ax in fig.colorbar so that if it is an iterable, even with just one element, it will be treated as a gridspec level colorbar. i.e. if instead of fig.colorbar(pc, ax=axs[2]) we make the axes a list:

fig.colorbar(pc, ax=[axs[2]])

we get:

(the user will still have to futz around w/ the colorbar aspect ratios to make that look nice).

This doesn't break constrained_layout=False (default), which both yeild identical layouts:

so this is a non-breaking change...

~~The todo list below is still active. I'll add a test and update the tutorial if this approach is OK.~~

Decided against a dedicated test. The underlying functionality works, so a test is a bit superfluous. Did update the tutorial.

PR Checklist

Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant

…idspec or subplotspec

codecov-io · 2018-02-28T18:00:01Z

Codecov Report

Merging #10629 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10629      +/-   ##
==========================================
+ Coverage   73.44%   73.44%   +<.01%     
==========================================
  Files         283      283              
  Lines       71729    71730       +1     
  Branches    10322    10322              
==========================================
+ Hits        52678    52679       +1     
  Misses      16136    16136              
  Partials     2915     2915

Impacted Files	Coverage Δ
lib/matplotlib/colorbar.py	`81.41% <100%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bb5994...3d64d65. Read the comment docs.

jklymak · 2018-02-28T19:13:36Z

New doc is: https://7023-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/tutorials/intermediate/constrainedlayout_guide.html#sphx-glr-tutorials-intermediate-constrainedlayout-guide-py

afvincent

The implementation looks good to me and this API change¹ seems to solve the surprising behavior reported in #10627.

But I would love to hear from a core dev with a bit more insights than I can provide on the right API for this 🐑.

¹ : which is a bit virtual as this is an experimental API ;)

afvincent · 2018-02-28T19:28:52Z

lib/matplotlib/axes/_subplots.py

@@ -173,7 +173,9 @@ def _make_twin_axes(self, *kl, **kwargs):
        """
        from matplotlib.projections import process_projection_requirements
        if 'sharex' in kwargs and 'sharey' in kwargs:
-            raise ValueError("Twinned Axes may share only one axis.")
+            if (not (kwargs['sharex'] == self ) and


Extra space before the parenthesis == self ): PEP8 seems to be unhappy.

afvincent · 2018-02-28T19:41:02Z

lib/matplotlib/axes/_subplots.py

-            raise ValueError("Twinned Axes may share only one axis.")
+            if (not (kwargs['sharex'] == self ) and
+                        not (kwargs['sharex'] == self)):
+                raise ValueError("Twinned Axes may share only one axis.")


Are those line a trial for a 2-in-1 PR done on purpose regarding to #10585 ?

Ooops, this shouldn't have been in this PR....

afvincent · 2018-02-28T19:50:38Z

tutorials/intermediate/constrainedlayout_guide.py

@@ -105,7 +105,7 @@ def example_plot(ax, fontsize=12, nodec=False):
 fig.colorbar(im, ax=ax, shrink=0.6)

 ############################################################################
-# If you specify multiple axes to the ``ax`` argument of ``colorbar``,
+# If you specify a list of axes to the ``ax`` argument of ``colorbar``,


If I understood correctly, even a tuple or an ndarray (or actually any kind of iterable...) will trigger that behavior, will it not? Should this be briefly mentioned or would it make the API even harder to understand :/?

Changed to a list (or other iterable container)

afvincent · 2018-02-28T19:53:52Z

tutorials/intermediate/constrainedlayout_guide.py

+
+############################################################################
+# The API to make a single-axes behave like a list of axes is to specify
+# it as a list, as below:


Similar “remark” about the word "list" (vs. a more general term to mean any kind of sequence) than above. TBH I do not have a clear opinion on this, but just wanted to make sure that one noticed the small subtlety.

jklymak · 2018-02-28T21:24:56Z

Thanks for the review @afvincent. Just to be clear about the API thing - I'm more than happy to have a different API for this, as this is a bit subtle. OTOH, it doesn't affect non-constrained_layout plots, so its a self-contained change. And because the API is experimental, if we don't like it, we can change it 😉

afvincent · 2018-02-28T23:20:23Z

Well, I have been too much exposed to Matplotlib and to its internals not to be biased, but ax=[axs[2]] is something that I actually tried (before this PR) when I was originally looking for a workaround. It was based on the loose intuition that a single Axes instance might be processed differently than a “group” of them. That is why IMO that API is not totally unreasonable ;). (But I would be glad as well if anybody would come suggesting another neat API.)

efiring

Makes sense to me.

…pec layout

Backport PR #10629 on branch v2.2.x

FIX/ENH Constrained Layout: Make whether single parent colorbar w/ gr…

3d64d65

…idspec or subplotspec

jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Feb 28, 2018

jklymak added this to the v2.2.0 milestone Feb 28, 2018

jklymak requested a review from afvincent February 28, 2018 18:51

afvincent approved these changes Feb 28, 2018

View reviewed changes

DOC: Update tutorial to explain colorbar API

acc4e17

jklymak force-pushed the fix-enh-colorbar-constrainedlayout branch from e729962 to acc4e17 Compare February 28, 2018 21:19

jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Mar 1, 2018

efiring approved these changes Mar 3, 2018

View reviewed changes

efiring merged commit e54939a into matplotlib:master Mar 3, 2018

lumberbot-app bot pushed a commit that referenced this pull request Mar 3, 2018

Backport PR #10629: FIX/ENH CL: Allow single parent colorbar w/ grids…

b49fcb2

…pec layout

lumberbot-app bot mentioned this pull request Mar 3, 2018

Backport PR #10629 on branch v2.2.x #10657

Merged

efiring mentioned this pull request Mar 3, 2018

Surprising alignment of (multiple) colorbars with constrained layout active #10627

Closed

jklymak added a commit that referenced this pull request Mar 3, 2018

Merge pull request #10657 from matplotlib/auto-backport-of-pr-10629

b67df2d

Backport PR #10629 on branch v2.2.x

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX/ENH CL: Allow single parent colorbar w/ gridspec layout #10629

FIX/ENH CL: Allow single parent colorbar w/ gridspec layout #10629

jklymak commented Feb 28, 2018 •

edited

Loading

codecov-io commented Feb 28, 2018

jklymak commented Feb 28, 2018

afvincent left a comment

afvincent Feb 28, 2018

afvincent Feb 28, 2018

jklymak Feb 28, 2018

afvincent Feb 28, 2018

jklymak Feb 28, 2018

afvincent Feb 28, 2018

jklymak commented Feb 28, 2018

afvincent commented Feb 28, 2018

efiring left a comment

FIX/ENH CL: Allow single parent colorbar w/ gridspec layout #10629

FIX/ENH CL: Allow single parent colorbar w/ gridspec layout #10629

Conversation

jklymak commented Feb 28, 2018 • edited Loading

PR Summary

PR Checklist

codecov-io commented Feb 28, 2018

Codecov Report

jklymak commented Feb 28, 2018

afvincent left a comment

Choose a reason for hiding this comment

afvincent Feb 28, 2018

Choose a reason for hiding this comment

afvincent Feb 28, 2018

Choose a reason for hiding this comment

jklymak Feb 28, 2018

Choose a reason for hiding this comment

afvincent Feb 28, 2018

Choose a reason for hiding this comment

jklymak Feb 28, 2018

Choose a reason for hiding this comment

afvincent Feb 28, 2018

Choose a reason for hiding this comment

jklymak commented Feb 28, 2018

afvincent commented Feb 28, 2018

efiring left a comment

Choose a reason for hiding this comment

jklymak commented Feb 28, 2018 •

edited

Loading