-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
axes collage #16603
Conversation
lib/matplotlib/figure.py
Outdated
For example :: | ||
|
||
x = [['A', 'A', 'B'], | ||
['C', 'D', 'B']] |
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.
do you also want to support
x = ['AAB',
'CDB']
?
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 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']]
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.
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.
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.
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
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.
if we support strings of arbitrary length, then how do we know that ["ABCD"] is ["A", "B", "C", "D"] vs ["AB", "CD"]?
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.
What's the deterministic parse rule on that? Single letter equals 1 subplot?
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.
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')]
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.
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.
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.
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:
- 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.
- 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.
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.
constrained_layout handles empty gridspec slots by placing invisible axes in the empty slots.
I wouldn't want autoexpansion of [['a'], ['b', 'c']] to [['a', 'a'], ['b', 'c']]. Perhaps could go in as provisional/unstable API? (I think we should try provisional APIs more :-)) |
Great! Quick thoughts:
|
Would the number just be labels or would the also be used to size the axes?
That is very clever and a good idea
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. |
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! |
see #15287 for post-creation sharing. |
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 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.
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
Two options come to mind here:
|
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.
Option 2 seems better (you can easily opt-in to sharing a sub-set of the axes) |
A dict is quite nice when working with complex layouts (at least personally I often stuff my axes in a dict anyways...).
and I guess we could even support sharing multiple axes at once ( |
bddeeb1
to
1ed1e3a
Compare
Implemented @ImportanceOfBeingErnest 's two suggestions and @jklymak 's internal API usage suggestions. |
Hey, great job! I wrote very similar code a couple days ago, in style of plt.subplots which allows you to simply do: I also accounted for None inputs. Have a look here and feel free to adopt if it suits you: |
Thanks @znstrider . |
07c80a2
to
e73c1a9
Compare
eba6a98
to
f132088
Compare
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 |
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. |
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.
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. 🤷
tutorials/provisional/mosaic.py
Outdated
# instead of an array. For comparison we could do: | ||
|
||
fig = plt.figure(constrained_layout=True) | ||
ax_array = fig.subplots(2, 2, squeeze=False) |
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.
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)
... 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 |
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.
Just a little be of copyediting in the tutorial, and a comment about the name of the helper function.
a56445f
to
23de96c
Compare
In answer to an earlier question: yes, squashing sounds to me like a good idea. |
23de96c
to
bbda124
Compare
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}"] |
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.
Probably should update Axes3D, too? Or just do a if hasattr(self, 'get_zlabel()')
or something?
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 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.
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 is awesome!
(I particularly like the nifty trick for ensuring axes labels are used for contiguous regions)
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.
Minor typos.
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'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 |
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.
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 |
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.
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 |
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.
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
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.
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='.'): |
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'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. |
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.
ASCII art
should be replaced with string
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.
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"?
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 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): |
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.
not thrilled that all the docs need to be repeated here, is it so help? works
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.
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.
tutorials/provisional/mosaic.py
Outdated
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) | ||
|
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 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.
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 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.
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 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.
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 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.
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.
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) |
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.
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) |
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.
Keep ABCD and just run the same code as above
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 think there is value in changing the letters up make clear that there is no actual meaning in the values of the letters.
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.
On more thought, we make that point later on, will change this to use D.
tutorials/provisional/README.txt
Outdated
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 |
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.
feedback at [mailing list/discourse/?]
f901793
to
5b5522b
Compare
subplot_kw = subplot_kw or {} | ||
gridspec_kw = gridspec_kw or {} | ||
# special-case string input |
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.
The internet says this is an antipattern...
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 is far from our only type-normalization on parsing input....
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 choked on that, too. It harks back to my Perl days, thankfully long gone.
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.
The choking episode was not fatal...
lib/matplotlib/figure.py
Outdated
|
||
def _do_layout(gs, layout, unique_ids, nested): | ||
""" | ||
Recursively do the lay out. |
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.
typo - layout is one word?
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.
Noun, as here: one word; but as a verb, one "lays out the plan".
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.
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>
5b5522b
to
424de9a
Compare
308 comments and 4 approvals--I think you set a record. |
🎉 Thank you everyone who contributed ideas and suggestions! The end result is an order of magnitude better than where it started! |
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
do what you expect.
open questions:
[['A'], ['B', 'C']]
<->[['A', 'A'][ ['B', 'C']]
)provide a way to have mixed subplot_kw to each of the created figuresto do laterPR Checklist