Skip to content
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

Compact string notation for subplot_mosaic #18731

Closed
timhoffm opened this issue Oct 14, 2020 · 13 comments · Fixed by #18763
Closed

Compact string notation for subplot_mosaic #18731

timhoffm opened this issue Oct 14, 2020 · 13 comments · Fixed by #18763
Milestone

Comments

@timhoffm
Copy link
Member

timhoffm commented Oct 14, 2020

Instead of

plt.subplot_mosaic(
    """
    AB
    CC
    """)

should we also allow a short string version

plt.subplot_mosaic("AB|CC")

?

Implementing is probably as simple as:

if '|' in layout and not '\n' in layout:
    layout = layout.replace('|', '\n')

to get back to the long version.

I got the idea while trying to find a compact replacement for the plot in https://matplotlib.org/devdocs/gallery/subplots_axes_and_figures/axes_margins.html.

@anntzer
Copy link
Contributor

anntzer commented Oct 14, 2020

I'm not against |, but note that you can already write "AB\nCC" which is not that bad...

@tacaswell
Copy link
Member

The vertical bar definitely scans better though, I'm mostly 👍, only concern is adding | to white space as things that may not be Axes names.

@story645
Copy link
Member

story645 commented Oct 15, 2020

I think if something gets added it should be keyword controlled like whitespace, but also wondering if at that point we're writing a patchwork expression evaluator (since | is vertical seperation and + is horizontal composition and I'm sure will be the next request from some folks even though it's implicit...)

@timhoffm
Copy link
Member Author

timhoffm commented Oct 15, 2020

The string layout definition already restricts names to single characters. Therefore, the limitation would only be that you cannot name an Axes | in that case. We don't need to impose a limitation on the list layout names.

The only use case I could come up with, would be layout='|-' but then again, you could also use layout='vh' (for vertical, horizontal). IMHO the possibility for a compact readable notation outweight a rather obscure use case of | as Axes name.

I don't think we should make everything configurable via keywords. This is a domain specific language and we're defining it. There's little point in allowing various dialects.

I don't think we need to make everything controllable via keywords. This a mini-language and we're defining it. There's little point in allowing various dialects (Not even sure we want to keep empty_sentinel in the long run).

As a side note, what we are doing is a bit different from patchwork expressions. Patchwork expressions are operators to compose elements (for the layout (A | B) / C), you need explicit operators because the elements are variables. However we just declare a layout (AB|CC) and have markers for empty and next row. We could use the slash for next row AB/CC because that's the row -compsition operator in patchwork. But IMO its reads a little worse and it's sort of a false friend as it's similar in semantic but a different concept, e.g. we won't have any of the other patchwork operators.

@story645
Copy link
Member

I read the patchworks docs wrong, and now I'm a little concerned that designating '|' as vertical when patchwork designates '|' as horizontal is gonna be very confusing.

@tacaswell
Copy link
Member

I am not super concerned about the dialect collision with patchwork, as @timhoffm says we are developing a mini declarative DSL that is sufficiently different we may not want to provide the false sense of similarity.

@tacaswell tacaswell added this to the v3.4.0 milestone Oct 15, 2020
@jklymak
Copy link
Member

jklymak commented Oct 15, 2020

I agree with most of @timhoffm points, but I'm mildly against a lot of parsing logic here. There is no need for this to grow more complex.

@story645
Copy link
Member

I am not super concerned about the dialect collision with patchwork, as @timhoffm says we are developing a mini declarative DSL that is sufficiently different we may not want to provide the false sense of similarity.

In this case though patchwork was a direct motivator and we expect overlapping user base so this seems like a point of confusion that we kind of can anticipate and I don't get walking directly into that conflict, especially since as Anthony points out \n works (and then we don't need to do an extra level of parsing.

@timhoffm
Copy link
Member Author

My primary concern is: Which specification reads best and is most intuitive for our average user:

plt.subplot_mosaic('AB|CC')
plt.subplot_mosaic('AB/CC')
plt.subplot_mosaic('AB\nCC')

In my opinion, the first one wins by large. The concerns raised above are valid but don't outweigh that advantage.

Re: Added complexity

The amount of extra work compared to the \n-version is negligible - probably one line of code and two lines of docs.

Re: Relation to patchwork

I acknowledge that moasic is inspired by one specific part of patchwork namely the textual layout mechanism. It's only that. We don't have anything to do with patchwork composition operators (also explicitly stated in the tutorial: "While we do not implement the operator overloading style, we do provide a Pythonic API for specifying (nested) Axes layouts." ).

Patchwork does not allow single-line layout strings, so there cannot be a strong expectation how the line separation char should look like. If I'd have to define single-line layout string for patchwork, the row composition operator would be a strong suggestion. However, we have our own layout specification, which is only loosely related to patchwork; e.g. we chose to use a different empty placeholder by default.

Given the association would only be indirect over two steps (mpl layout --> patchwork layout --> patchwork compostion operator), I don't by that patchwork users would be significantly confused. Moreover, only a minority of our users will know patchwork anyway.

@story645
Copy link
Member

story645 commented Oct 16, 2020

Moreover, only a minority of our users will know patchwork anyway.

It'd be the R users who already tend to be (understandably) frustrated that we have a different paradigm. I very much worry about the frustration of '|' means vertical alignment here and 'I' means horizontal alignment in the closest analogue they also might be using concurrently.

What about borrowing from the other layout and using "," so "AA,BB"?

@timhoffm
Copy link
Member Author

',' is easy to mix up with '.', which is our empty sentinel. A better variant would be

plt.subplot_mosaic('AB;CC')

in analogy to multiple statements on the same line.

@story645
Copy link
Member

I love the ';', it gets my vote!

@timhoffm
Copy link
Member Author

timhoffm commented Oct 16, 2020

Reading below this line is at own personal risk. Just noted down some crazy ideas so that they don't get lost.

This is not an official API propsal.


At the danger of annoying everybody here again: ; as row separator almost calls for , as (optional) column separtor and allowing multi-char names in strings. At least for simple cases that'd be nice:

plt.subplot_mosaic('top;bottom')
plt.subplot_mosaic('left,right')
plt.subplot_mosaic('left,right;bottom,bottom')

Oh, or alternatively mapping single chars to long names:

plt.subplot_mosaic('AB;CC', names={'A': 'left', 'B': 'right', 'C': 'bottom'})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants