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

Figure builder class feature. #2059

Open
neuralsignal opened this issue May 8, 2020 · 9 comments
Open

Figure builder class feature. #2059

neuralsignal opened this issue May 8, 2020 · 9 comments

Comments

@neuralsignal
Copy link

Hi,

Thank you for this wonderful tool!

I recently forked the project (https://github.com/gucky92/seaborn.git), and have been playing around a bit.
I have added two main features to my fork and wanted to know if there is general interest or consideration to implement something similar.

First, I made some changes to the Grid classes to allow passing a figure and subplot_spec argument in the init. Thus, a Grid figure can be part of a larger figure using gridspec.GridSpecFromSubplotSpec. This also allowed me to build a Figure builder class that can include individual subplots and Grid plots. This Figure builder class also allows for adding panel letters to each axis.

Second, I added a scalebar function that uses some of the seaborn functionality to convert the x-axis and/or y-axis to a scalebar. I added a method to the Grid class to convert the axes internally to scalebars.

Here is a MyBinder with quick examples of the two features:
Binder

Please let me know what you think!

@neuralsignal
Copy link
Author

I see something similar has been discussed and implemented: 339

@mwaskom
Copy link
Owner

mwaskom commented May 8, 2020

Hi, yes, people have asked before about inserting multi-axes seaborn plots into a larger matplotlib figure. The problem is that too much of the logic of the seaborn Grid objects assumes that they have complete control of their figure. A few immediate issues with no clear solution:

  • This approach ignores the height= and aspect= parameters of the Grid objects. Maybe that's fine, but it's still an important API decision that should be made carefully.
  • What happens when you add a legend to a FacetGrid or PairGrid? Those objects expand the figure they live in to place the legend outside of the grid of axes properly. You don't show an example of a legend (even though your PairGrid has a hue semantic) but I don't see how it could work with the approach of initializing a Gridobject within a specific SubplotSpec

The assumption of figural control is also key to planned improvements to these objects: #2051. I expect that implementing the planned changes would further break compatibility with your approach.

As for the other changes: I don't see why a function for adding panel letters needs to be a method on a dedicated seaborn class; it should be possible to write an external function that consumes an arbitrary matplotlib figure and adds labels based on the positions of its subplots. The same is true for the scale bars. (Also these might just be personal opinions but (a) I don't love the x/y scalebars that touch at the origin; it's too confusing because they look like normal spines (b) I don't like having a scalebar on every axes of a small multiples plot).

Finally, just as a suggestion about development, I see you're adding a bunch of unrelated changes onto the master branch of your fork. If any of these were to get incorporated upstream they'd have to be in feature-specific branches, and you're setting yourself up for a real git headache by developing this way.

@neuralsignal
Copy link
Author

Thank you for your input!

  • Yes, height and aspect parameters are being ignored currently. I might be able to incorporate them in some way in the figure builder class.
  • Yeah I don't know how to handle legends yet. Still playing around with that issue. I am keeping the subplot_spec as an attribute for each grid instance; maybe using the subplot_spec I can place the legend properly. For example,
    I will try to keep track with your planned improvements, so that compatibility is not broken, and let you know if I find a good solution.

Yeah sorry for my messy master branch. I am new to open source development. I created a new branch with only the Figure builder class and the changes to the Grid classes: https://github.com/gucky92/seaborn/tree/figure_build
I removed all the other features from this branch.

@mwaskom
Copy link
Owner

mwaskom commented May 8, 2020

Here's another concern with this specific approach: by having your Figure class define add_facetgird, etc., it doesn't seem possible to use figure-level seaborn plotting functions (relplot, catplot, etc.). These functions exist because using FacetGrid directly with seaborn functions that do a lot of automatic inference can be complex and error-prone; going forward I think the documentation is going to change to emphasize that direct usage of FacetGrid is generally not needed and is recommended only when you need a custom plot and really know what you're doing.

So, if there were going to be a change that allowed placing Grid objects within an existing figure, it would have to support existing "figure-level" functions, which aligns much better with the previously suggested approach of accepting a subplot_spec argument in Grid objects, similar to how axes-level functions accept ax=.

@neuralsignal
Copy link
Author

I also accept the subplot_spec argument in Grid, so it would work with relplot and other figure-level seaborn plotting functions. I just built the Figure builder class for convenience since it allowed me to quickly create my figures. I can easily add methods for adding a relplot to the Figure builder class.

@mwaskom
Copy link
Owner

mwaskom commented May 8, 2020

Also, the text in the previous issue emphasizes an important challenge: the notion of "figure-level" and "axes-level" as the core taxonomy of seaborn functions. These concepts are emphasized strongly in the documentation and elsewhere. Changing the Grid objects to optionally live in an intermediate zone ("subplotspec-level"? matplotlib doesn't have a great taxonomy here either) will breed confusion.

That kind of meta-level argument doesn't necessarily have to block really compelling features, but it does mean that it would be a more substantial change/commitment than might be immediately apparent.

@neuralsignal
Copy link
Author

Thanks. Yes, it will probably be a more substantial commitment. What would be the best approach going forward, if I have time to rigorously implement such as feature?

Finally, at the risk of becoming Nietzschean, maybe a figure builder class can be viewed as a "superfigure".

@mwaskom
Copy link
Owner

mwaskom commented May 8, 2020

My suggestion would probably be to push forward on an approach that assembles a super-figure from existing figure objects. That approach has been prototyped here: https://stackoverflow.com/questions/35042255/how-to-plot-multiple-seaborn-jointplot-in-subplot.

Note that this is similar (in my understanding) to how patchwork works w/r/t ggplot, so there is precedent there.

A third-party solution would avoid many of the concerns I've raised above (you consume the figure after the Grid objects have done all of the expansion they need to do, you could create the super figure such that the size/aspect ratio of the sub-grids is preserved, and you don't have to muddle the taxonomy of objects in seaborn itself). It would also mean that you'd own the maintenance burden, for better or worse.

@jklymak
Copy link
Contributor

jklymak commented Jan 6, 2023

Curious where seaborne was on this - Matplotlib now has subfigures so passing an optional (sub)figure kwarg to jointplot and other figure-level artists should just work.

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

No branches or pull requests

3 participants