Skip to content

axes collage #16603

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

Merged
merged 1 commit into from
Jun 13, 2020
Merged

axes collage #16603

merged 1 commit into from
Jun 13, 2020

Conversation

tacaswell
Copy link
Member

@tacaswell tacaswell commented Feb 28, 2020

PR Summary

This is an attempt at implementing a nicer API for laying out complex axes schemes like patchwork for ggplot. This is following on from a conversation @anntzer and @story645 had a few weeks ago
https://hackmd.io/4zWKhuLXQ16Y_oUfi9hnKA#Gridspec-Easy-API and was done on a red-eye flight home

The big win here is things like

x = [["A", "A", "B"], ["C", "D", "B"]]
fig = plt.figure()
axes = fig.build_grid(x)
axes['A'].plot(...)

do what you expect.

open questions:

  • Name (Hannah suggested something riffing to "collage" which I think is good if we can make it a verb)
  • does this go in Matplotib or does this go in a small library under the mpl org?
  • better tests of things going wrong
  • be sure that when re-assigning grid specs we clean up all the evidence of the old one (to be sure constrained layout works etc)
  • do we want the expansion logic to work ([['A'], ['B', 'C']] <-> [['A', 'A'][ ['B', 'C']])
  • keys in return dict when adjusting existing axes
  • provide a way to have mixed subplot_kw to each of the created figures to do later

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

For example ::

x = [['A', 'A', 'B'],
['C', 'D', 'B']]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you also want to support

x = ['AAB',
     'CDB']

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested that and the argument @story645 had against it is that we should be encouraging long names like

x = [['map', 'histogram'], 
     ['time_series', 'time_series']]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but, that said, I suspect that could be implemented in the same pre-processing of the input that we would need to do for the auto-expanding logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that still works, the point is to support List[str] vs List[List[str]]
I don't have a really strong opinion there though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we support strings of arbitrary length, then how do we know that ["ABCD"] is ["A", "B", "C", "D"] vs ["AB", "CD"]?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the deterministic parse rule on that? Single letter equals 1 subplot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you either pass in a single string or a list.

    @staticmethod
    def _normalize_grid_string(layout):
        return [list(ln) for ln in layout.strip().split('\n')]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the point I could be clearer in making is the docs have to be 100 percent explicit that for string input, each character (excluding new lines and , ) is parsed as a subplot.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all - thanks for looking at that gist - would love to see this in matplotlib. I'd definitely encourage you to include the multi-line string approach because even though it's not super-generalisable it covers a very common use case for scientists preparing figures.

Two other random notes:

  1. I updated my gist so that you can now use a '.' to indicate an empty axis because then I don't have to worry about spaces.
  2. If you can find a good way to add the panel labels so that they align correctly with tight_layout even with different axis sizes without having to do any fiddling, that would be amazing. There might already be a good way of doing this but I've never figured it out. My current solution in the gist sort of works but is a bit hacky and the alignment isn't quite right (the letters should line up on the left not the right).

Thanks for all your work on matplotlib - it's very much appreciated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constrained_layout handles empty gridspec slots by placing invisible axes in the empty slots.

@anntzer
Copy link
Contributor

anntzer commented Feb 28, 2020

I wouldn't want autoexpansion of [['a'], ['b', 'c']] to [['a', 'a'], ['b', 'c']].
Didn't check all the edge cases but I think the base idea is fine. I like returning the dict (makes it easy to manage plenty of axes semantically).

Perhaps could go in as provisional/unstable API? (I think we should try provisional APIs more :-))

@ImportanceOfBeingErnest
Copy link
Member

Great! Quick thoughts:

  1. Could it also take arrays of numbers in, instead of strings? Those would be much easier to generate.

  2. Can it take None to create an empty space? I.e.

    [[None, "A", "A", None],
    ["B", "B", "C", "C"],
    [None, "D", "D", None]]
    

    creating
    image

  3. Should it allow for sharing? In the sense of allowing to share "A", with "B", but not with "C"?

@tacaswell
Copy link
Member Author

Could it also take arrays of numbers in, instead of strings? Those would be much easier to generate.

Would the number just be labels or would the also be used to size the axes?

Can it take None to create an empty space? I.e.

That is very clever and a good idea

Should it allow for sharing? In the sense of allowing to share "A", with "B", but not with "C"?

I don't see any way to spell that API that does not turn into an unusable knot very quickly. Maybe we should push on (or just document if it is already possible and I don't know how to do it) to set up the shared axes after the fact.

@tacaswell
Copy link
Member Author

I wrote this up on the plane because it was fun (I don't write much actual code anymore).

There was some question about how hard this would be (in my head) and some question if people think it is a good idea. With @story645 @anntzer and @ImportanceOfBeingErnest on board I think we have the answer to the second question and this code is too bad so I think we have the answer on the first.

If someone is super excited about this and wants to take over leading / championing its implementation feel free to do so!

@anntzer
Copy link
Contributor

anntzer commented Feb 28, 2020

see #15287 for post-creation sharing.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems workable as long as the scope doesn't get out of control. As soon as you do this, people are going to want to add colorbar positioning, etc etc.

Not sure about the return dictionary versus just a list. I guess you have to do it this way so the order is unambiguous? It is a bit unexpected to have to deal with a dictionary, but maybe it has advantages too.

@ImportanceOfBeingErnest
Copy link
Member

Could it also take arrays of numbers in, instead of strings? Those would be much easier to generate.

Would the number just be labels or would the also be used to size the axes?

I don't see how to use numbers for sizes in a reasonable way, so this is really just a question of whether the user is expected to do np.array(...).astype(str) themselves or not.
For sizes one would probably need to fall back to the gridspec kw width_ratios and height_ratios.

Should it allow for sharing? In the sense of allowing to share "A", with "B", but not with "C"?

I don't see any way to spell that API that does not turn into an unusable knot very quickly.

Two options come to mind here:

  1. Another matrix with unique identifiers of shared axes, of the same shape as the input matrix, e.g.

    x = [["A", "A", "B"], ["C", "D", "B"]]
    sharex = [["U", "U", "V"], ["V", "U", "V"]]
    fig.build_grid(x, sharex=sharex)
    

    would share the x axes of A and D, as well as B and C .

  2. A list of shared axes identifiers, e.g.

    x = [["A", "A", "B"], ["C", "D", "B"]]
    sharex = [["A", "D"], ["B",  "D"]]
    fig.build_grid(x, sharex=sharex)
    

    for the same effect as above.

@tacaswell
Copy link
Member Author

Not sure about the return dictionary versus just a list. I guess you have to do it this way so the order is unambiguous? It is a bit unexpected to have to deal with a dictionary, but maybe it has advantages too.

Once things are layout out in a complex ways it is not clear to me that there is an un-ambiguous order through them. Returning a dictionary is also stable under adding nesting layout and adding / removing other axes from the layout.

Two options come to mind here:

Option 2 seems better (you can easily opt-in to sharing a sub-set of the axes)

@anntzer
Copy link
Contributor

anntzer commented Feb 29, 2020

A dict is quite nice when working with complex layouts (at least personally I often stuff my axes in a dict anyways...).
Re: sharing: After #15287 you could just do

ax["A"].sharex(ax["B"])

and I guess we could even support sharing multiple axes at once (ax["A"].sharex([ax["B"], ax["C"]])), so I'm not sure this really warrants direct support in build_grid (in particular, I'm not sure the suggested solution is more readable...).

@tacaswell
Copy link
Member Author

Implemented @ImportanceOfBeingErnest 's two suggestions and @jklymak 's internal API usage suggestions.

@znstrider
Copy link
Contributor

Hey, great job!

I wrote very similar code a couple days ago, in style of plt.subplots which allows you to simply do:
fig, ax = gs_from_array(layout)

I also accounted for None inputs.

Have a look here and feel free to adopt if it suits you:
https://gist.github.com/znstrider/a3b915f38e27e480640d695ac342adc1

@tacaswell
Copy link
Member Author

Thanks @znstrider . np.argwhere definitely makes it simpler!

@tacaswell tacaswell added this to the v3.3.0 milestone Apr 22, 2020
@efiring
Copy link
Member

efiring commented Jun 9, 2020

Well, I hate to suggest even more work, and ripping out existing work, but in fact I'm inclined to agree with @timhoffm. The rearranging of existing Axes by this function seems a bit bizarre and pointless. In the tutorial example, one would wonder why subplots was used in the first place, and I have no idea, looking at the example code but not the resulting figure, what the 'tight_layout' kwarg is doing in this case.

@jklymak
Copy link
Member

jklymak commented Jun 9, 2020

I suspect it comes from the R/ggplot/patchwork paradigm where the plots are made first and then arranged. However, I think its a foot cannon, and agree it should be removed.

For example, imagine someone has attached a colorbar to the multiple axes. What does calling this after the fact do to the colorbar layout? I'm guessing nothing good.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time I read over this, I have some new comments. At some point I should stop as this is good enough for a start. 🤷

# instead of an array. For comparison we could do:

fig = plt.figure(constrained_layout=True)
ax_array = fig.subplots(2, 2, squeeze=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe go explicit for this case:

# subplots
fig1 = plt.figure(constrained_layout=True)
ax_array = fig1.subplots(2, 2, squeeze=False)

# subplot_mosaic
fig2 = plt.figure(constrained_layout=True)
ax_dict = fig2.subplot_mosaic(layout)

@jklymak
Copy link
Member

jklymak commented Jun 10, 2020

However, I think its a foot cannon, and agree it should be removed.

... or soft deprecated by not mentioning it in the docs?

@tacaswell
Copy link
Member Author

or soft deprecated by not mentioning it in the docs?

You can't soft deprecate something that does not exist yet :-p We soft-deprecate by de-documenting things we think are a bad idea, but know we have users of that we can't unilaterally break. If the consensus is it is a bad idea lets not put it in. I thought it was a cute / clever functionality but that may be a mark against it....

If I'm going to take a stand for one bit of extra complexity I'd rather keep the empty_sentinel kwarg.

Copy link
Member

@WeatherGod WeatherGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little be of copyediting in the tutorial, and a comment about the name of the helper function.

@efiring
Copy link
Member

efiring commented Jun 10, 2020

In answer to an earlier question: yes, squashing sounds to me like a good idea.

@tacaswell
Copy link
Member Author

Squashed to one commit.

if self.get_xlabel():
fields += [f"xlabel={self.get_xlabel()!r}"]
if self.get_ylabel():
fields += [f"ylabel={self.get_ylabel()!r}"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should update Axes3D, too? Or just do a if hasattr(self, 'get_zlabel()') or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have a custom repr on the Axes 3D than to inject knowledge of it here. The subtle but pervasive interconnection is one of the things that makes maintaining Matplotlib hard.

Copy link
Member

@WeatherGod WeatherGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

(I particularly like the nifty trick for ensuring axes labels are used for contiguous regions)

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typos.

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that the tutorial abstracts away actually working w/ the graphs and what we want to actually show repeatedly is how axes identification works w/ this new feature.

@@ -0,0 +1,24 @@
Add patch-work inspired API for generating grids of Axes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree w/ jody needs to be labeled Provisional & like needs "semantic/named" in here for the folks who don't know what patchwork is.

Add patch-work inspired API for generating grids of Axes
--------------------------------------------------------

The `.Figure` class has a provisional method to easily generate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when possible avoid words like easily - either the example will seem easy to folks or it won't


axd = plt.figure(constrained_layout=True).subplot_mosaic(
"""
AAE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in all honesty the single character version of the API horrifies me so I'd really rather we use the nested list "['clear name 1", "clear name 2"] version or at the least also highlight it in the what's new

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an API point of view I agree, but what I learned writing the tutorial is that the string based one is several times easier to use. Added an example of the list one to this.

return [list(ln) for ln in layout.strip('\n').split('\n')]

def subplot_mosaic(self, layout, *, subplot_kw=None, gridspec_kw=None,
empty_sentinel='.'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👍 on empty_sentinel kwarg

def subplot_mosaic(self, layout, *, subplot_kw=None, gridspec_kw=None,
empty_sentinel='.'):
"""
Build a layout of Axes based on ASCII art or nested lists.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASCII art should be replaced with string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has type str, but it has extra meaning (it should look like the layout you want) which I think is captured by ASCII art. I don't like that it suggests only ASCII will work, but I don't think anyone would understand "unicode-art"?

Copy link
Member

@story645 story645 Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think art implies well art and that's kind of idiomatic/metaphorical when it's literally a string of characters.

@@ -1274,6 +1274,86 @@ def subplots(nrows=1, ncols=1, sharex=False, sharey=False, squeeze=True,
return fig, axs


def subplot_mosaic(layout, *, subplot_kw=None, gridspec_kw=None,
empty_sentinel='.', **fig_kw):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not thrilled that all the docs need to be repeated here, is it so help? works

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also a slightly different API because we also create the figure. Having the duplication is annoying, but I don't think there is a mechanical way to do the re-writing that is simpler in the long run that just having the duplicate text.

Comment on lines 41 to 63
def identify_axes(ax_dict, fontsize=48):
"""
Helper to identify the Axes in the examples below.

Draws the label in a large font in the center of the Axes.

Parameters
----------
ax_dict : Dict[str, Axes]
Mapping between the title / label and the Axes.

fontsize : int, optional
How big the label should be
"""
kw = dict(ha='center', va='center', fontsize=fontsize, color='darkgrey')
for k, ax in ax_dict.items():
ax.text(0.5, 0.5, k, **kw)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes the tutorial really really hard to follow along 'cause it's kinda magiking away the actual plotting w/ the axes. I think this tutorial would benefit from verbosity to make the two methods super clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much disagree on this. The point of this tutorial is to show how to use the subplot_mosaic API to generate layouts of Axes. By putting the label is huge text in the middle it makes the mapping between the input the subplot_mosaic and the layout clear. Doing actually plotting on the Axes will be distracting from the main goal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cry and get super-frustrated any time I see functions in the docs 'cause it requires me to keep a call stack in my head.
Also I think we have cross purposes here. theidentify_axes is good for showing how subplot_mosaic works because it prints the layout, yes. It's not great at showing how to use subplot_mosaic because it abstracts away the usage of the axes which is integral to why I should use this method. What about a combination of identify_axes to put text in the background, but then put plotting on top.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should have a simple example at the beginning in the "usual" workflow, including actual plot commands. Then this should be followed up with a simple example using this new method of creating the axes, again with the simple plot commands. After that, there is no need to keep showing plotting commands for showing the features of the method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conversely, docs that have almost the same boiler plate repeated multiple times is also very frustrating to use because instead of "and a function is called" you have to mentally do the diff of 15 lines of code while scrolling between the examples to sort out what was actually changed. By putting the stuff that is the same in a function the signal to noise goes way up.

I'm not really convinced that this change adds anything, but I also don't think it does any harm.

I re-organized the tutorial to start with subplots + plotting -> list with nice names + plotting -> string short hand -> the extra kwargs ->


fig = plt.figure(constrained_layout=True)
ax_dict = fig.subplot_mosaic(layout)
identify_axes(ax_dict)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kw = dict(ha='center', va='center', fontsize=fontsize, color='darkgrey')
fig, axd=  plt.subplot_mosaic(layout, constrained_layout=True)
axd['A'].text(0.5, 0.5, "A", **kw)
axd['B'].plot([1,2,3])
axd['C'].hist(range(10)]
axd.['D'].imshow([[1,2],[2,1]])

this is equivalent to

fig, axes=  plt.subplots(nrows=2, ncols=2, constrained_layout=True)
axes[0,0].text(0.5, 0.5, "A", **kw)
axes[[0,1].plot([1,2,3])
axes[[1,0].hist(range(10)]
axes.[1,1].imshow([[1,2],[2,1]])

CCE
"""
)
identify_axes(axd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep ABCD and just run the same code as above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is value in changing the letters up make clear that there is no actual meaning in the values of the letters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On more thought, we make that point later on, will change this to use D.

These tutorials cover proposed APIs of any complexity. These are here
to document features that we have released, but want to get user
feedback on before committing to them. Please have a look, try them
out and give us feedback! But, be aware that we may change the APIs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feedback at [mailing list/discourse/?]

@tacaswell tacaswell force-pushed the enh_subplot_grid branch 4 times, most recently from f901793 to 5b5522b Compare June 12, 2020 20:40
Comment on lines +1595 to +1597
subplot_kw = subplot_kw or {}
gridspec_kw = gridspec_kw or {}
# special-case string input
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internet says this is an antipattern...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is far from our only type-normalization on parsing input....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I choked on that, too. It harks back to my Perl days, thankfully long gone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The choking episode was not fatal...


def _do_layout(gs, layout, unique_ids, nested):
"""
Recursively do the lay out.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo - layout is one word?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noun, as here: one word; but as a verb, one "lays out the plan".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

force pushed a fix.

See tutorials/provisional/mosaic.py for details.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Benjamin Root <ben.v.root@gmail.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@efiring efiring merged commit f80d56e into matplotlib:master Jun 13, 2020
@efiring
Copy link
Member

efiring commented Jun 13, 2020

308 comments and 4 approvals--I think you set a record.

@tacaswell tacaswell deleted the enh_subplot_grid branch June 13, 2020 03:02
@tacaswell
Copy link
Member Author

🎉

Thank you everyone who contributed ideas and suggestions! The end result is an order of magnitude better than where it started!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.