| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| For example :: | ||
|
|
||
| x = [['A', 'A', 'B'], | ||
| ['C', 'D', 'B']] |
There was a problem hiding this comment.
do you also want to support
x = ['AAB',
'CDB']
?
Sorry, something went wrong.
There was a problem hiding this comment.
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']]
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
if we support strings of arbitrary length, then how do we know that ["ABCD"] is ["A", "B", "C", "D"] vs ["AB", "CD"]?
Sorry, something went wrong.
There was a problem hiding this comment.
What's the deterministic parse rule on that? Single letter equals 1 subplot?
Sorry, something went wrong.
There was a problem hiding this comment.
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')]
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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:
Thanks for all your work on matplotlib - it's very much appreciated.
Sorry, something went wrong.
There was a problem hiding this comment.
constrained_layout handles empty gridspec slots by placing invisible axes in the empty slots.
Sorry, something went wrong.
|
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 :-)) |
Sorry, something went wrong.
|
Great! Quick thoughts:
|
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
|
see #15287 for post-creation sharing. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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.
Two options come to mind here:
|
Sorry, something went wrong.
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) |
Sorry, something went wrong.
|
A dict is quite nice when working with complex layouts (at least personally I often stuff my axes in a dict anyways...). 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...). |
Sorry, something went wrong.
|
Implemented @ImportanceOfBeingErnest 's two suggestions and @jklymak 's internal API usage suggestions. |
Sorry, something went wrong.
|
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: |
Sorry, something went wrong.
|
Thanks @znstrider . np.argwhere definitely makes it simpler! |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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. 🤷
Sorry, something went wrong.
| # 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.
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)
Sorry, something went wrong.
... or soft deprecated by not mentioning it in the docs? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Just a little be of copyediting in the tutorial, and a comment about the name of the helper function.
Sorry, something went wrong.
|
In answer to an earlier question: yes, squashing sounds to me like a good idea. |
Sorry, something went wrong.
|
Squashed to one commit. |
Sorry, something went wrong.
| 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.
Probably should update Axes3D, too? Or just do a if hasattr(self, 'get_zlabel()') or something?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
This is awesome!
(I particularly like the nifty trick for ensuring axes labels are used for contiguous regions)
Sorry, something went wrong.
There was a problem hiding this comment.
Minor typos.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| @@ -0,0 +1,24 @@ | |||
| Add patch-work inspired API for generating grids of Axes | |||
There was a problem hiding this comment.
Agree w/ jody needs to be labeled Provisional & like needs "semantic/named" in here for the folks who don't know what patchwork is.
Sorry, something went wrong.
| 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.
when possible avoid words like easily - either the example will seem easy to folks or it won't
Sorry, something went wrong.
|
|
||
| axd = plt.figure(constrained_layout=True).subplot_mosaic( | ||
| """ | ||
| AAE |
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| 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.
I'm 👍 on empty_sentinel kwarg
Sorry, something went wrong.
| 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.
ASCII art should be replaced with string
Sorry, something went wrong.
There was a problem hiding this comment.
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"?
Sorry, something went wrong.
There was a problem hiding this comment.
I think art implies well art and that's kind of idiomatic/metaphorical when it's literally a string of characters.
Sorry, something went wrong.
|
|
||
|
|
||
| def subplot_mosaic(layout, *, subplot_kw=None, gridspec_kw=None, | ||
| empty_sentinel='.', **fig_kw): |
There was a problem hiding this comment.
not thrilled that all the docs need to be repeated here, is it so help? works
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| 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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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 ->
Sorry, something went wrong.
|
|
||
| fig = plt.figure(constrained_layout=True) | ||
| ax_dict = fig.subplot_mosaic(layout) | ||
| identify_axes(ax_dict) |
There was a problem hiding this comment.
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]])
Sorry, something went wrong.
| CCE | ||
| """ | ||
| ) | ||
| identify_axes(axd) |
There was a problem hiding this comment.
Keep ABCD and just run the same code as above
Sorry, something went wrong.
There was a problem hiding this comment.
I think there is value in changing the letters up make clear that there is no actual meaning in the values of the letters.
Sorry, something went wrong.
There was a problem hiding this comment.
On more thought, we make that point later on, will change this to use D.
Sorry, something went wrong.
| 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.
feedback at [mailing list/discourse/?]
Sorry, something went wrong.
| subplot_kw = subplot_kw or {} | ||
| gridspec_kw = gridspec_kw or {} | ||
| # special-case string input |
There was a problem hiding this comment.
The internet says this is an antipattern...
Sorry, something went wrong.
There was a problem hiding this comment.
This is far from our only type-normalization on parsing input....
Sorry, something went wrong.
There was a problem hiding this comment.
I choked on that, too. It harks back to my Perl days, thankfully long gone.
Sorry, something went wrong.
There was a problem hiding this comment.
The choking episode was not fatal...
Sorry, something went wrong.
|
|
||
| def _do_layout(gs, layout, unique_ids, nested): | ||
| """ | ||
| Recursively do the lay out. |
There was a problem hiding this comment.
typo - layout is one word?
Sorry, something went wrong.
There was a problem hiding this comment.
Noun, as here: one word; but as a verb, one "lays out the plan".
Sorry, something went wrong.
There was a problem hiding this comment.
force pushed a fix.
Sorry, something went wrong.
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>
|
308 comments and 4 approvals--I think you set a record. |
Sorry, something went wrong.
|
🎉 Thank you everyone who contributed ideas and suggestions! The end result is an order of magnitude better than where it started! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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:
PR Checklist