Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Adding subfigure_mosaic#26061

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
turnipseason wants to merge9 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromturnipseason:add-subfigure-mosaic

Conversation

turnipseason
Copy link
Contributor

@turnipseasonturnipseason commentedJun 3, 2023
edited
Loading

PR summary

Resolves#25949

Made initial changes:

  • Refactoredsubplot_mosaic
  • Added_sub_prep method that contains thesubplot_mosaic logic
  • Addedsubfigure_mosaic

Still needs to be done:

  • Documentation
  • Tests
  • Examples

PR checklist

@turnipseason
Copy link
ContributorAuthor

I have a question about **kwargs. Am I correct in thinking that we should be using that inadd_subplot andadd_subfigure, to pass tosub_prep? I got a bit confused and decided to leave things explicit, but this seems like it should be done differently.

@rcomer
Copy link
Member

I think what you have done makes sense._sub_prep does the checking that there are no common keys insubthing_kw andper_subthing_kw, so it needs to have those dictionaries separate. Or have I misunderstood your question?

@turnipseason
Copy link
ContributorAuthor

@rcomer Thank you for answering!
Maybe I misunderstood the way kwargs processing works. Fom what I gather, we're not using the kwargs that we pass tosubplot_mosaic andsubfigure_mosaic directly. Instead, we're passing them further for_sub_prep to use. So we should be doing something like:

defsubfigure_mosaic(self,mosaic,**kwargs):ret=self._sub_prep(mosaic,self.add_subfigure,kwargs)

instead of what is currently happening. But the definition of_sub_prep should remain the same (explicit key/value definitions) because we're using the kwargs in it.
I had difficulties in implementing it that way though, so I thought I'd get feedback before changing anything.

}

per_subplot_kw = self._norm_per_subplot_kw(per_subplot_kw)
per_subthing_kw = self._norm_per_subplot_kw(per_subthing_kw)
Copy link
Member

Choose a reason for hiding this comment

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

Does this normalization also need to be changed?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Do you mean changing the name to_norm_per_subthing_kw or changing the internal implementation? I checked and it seems to be working both foradd_subplot_mosaic andadd_subfigure_mosaic.

Copy link
Member

Choose a reason for hiding this comment

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

ah, just went and checked the source, this is a class method that is making sure that the multiple ways to reference a sub-object do not end up trying to address a given element more than once.

The classmethod needs a better name (I named it and the current name is....bad), maybe_check_for_subthing_kw_conflicts ?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

My first thought was_flatten_subthing_kw but your version sounds more descriptive of what the method's actually being used for. Should I go ahead and change it to that?

Copy link
Member

Choose a reason for hiding this comment

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

flatten is also pretty good (maybe better). In general, the prior should be "Tom is bad at naming things".

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Is something like_check_duplication_and_flatten_subthing_kw too long?

Copy link
Member

Choose a reason for hiding this comment

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

it is an internal method, so no ;)

Copy link
ContributorAuthor

@turnipseasonturnipseasonJun 8, 2023
edited
Loading

Choose a reason for hiding this comment

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

Got you :) I'll change it to that if you're ok with it, then.

Edit: this was two symbols too long for flake8, so I renamed it to_check_duplication_and_flatten_kwargs

@tacaswell
Copy link
Member

The reason we do not use**kwargs but instead havesubplot_kw andgridspec_kw is that both of those things have "claim" to the keywords, however we can not reliably separate which keys / values should go to which. In part because one of the keywords that can go toadd_subplot asaxes_class which lets the user switch out the Axes class that will be used and those (sub) classes can have arbitrary keywords (😱) thus there may be a collision withgridspec_kw. Wecould have picked priority in favor ofgridspec_kw, but we opted for being explicit.

Currentlyadd_subfigure does not have that level of flexibility. However I think we should rhyme with the subplot API as much as possible, preserve the ability to add the flexibility in the future, and remain explicit about where internally the keywords are being routed.

turnipseason reacted with thumbs up emoji

@tacaswell
Copy link
Member

It looks like the mosaic tests all passed so that is a good sign!

@tacaswelltacaswell added this to thev3.8.0 milestoneJun 8, 2023
@tacaswell
Copy link
Member

Code is looking good, now it needs docs, examples and whats new :)

@turnipseason What are the odds of you having time to get this done in the next two weeks? We are getting ready for the 3.8 release and it would be great to get this in for that!

@turnipseason
Copy link
ContributorAuthor

@tacaswell I've been a bit busy the past week but now I think I will have the time to do this!

melissawm reacted with hooray emoji

@turnipseason
Copy link
ContributorAuthor

turnipseason commentedJun 26, 2023
edited
Loading

@tacaswell Hi! I've been trying to write small examples and discovered some problems.

  1. The test for "nested" layouts (test_nested) passes, but, for example, if we compare the result to that ofsubplot_mosaic, we can see that it's wrong. It seems like this only happens when having inner lists within other lists, like so:
inner= [ ["inner A"], ["inner B"], ]outer_nested_mosaic= [["main",inner], ["bottom","bottom"],]axda=plt.figure(layout="constrained").subfigure_mosaic(outer_nested_mosaic)

This (with some keywords for clarity + identify_axes) gives us:

Whereassubplot_mosaic with the same parameters looks like:

Is this expected? Should I try to fix this behavior or just write into the docs that it shouldn't be used like that?

  1. The second thing is that there are two more tests forsubplot_mosaic that test height and width ratios in nested list situations, using the.get_gridspec(). The first problem is as described above. The second is that for SubFigures that requires calling.gca() but that always results in a (1,1) GridSpec.
    Thetest_nested passes, so just making it an image comparison wouldn't make sense. Is there a different way of doing this?

  2. With regards to the examples themselves -- I'm leaning more towards writing a separate file that mimicsthis with elements fromthis, as you said. Should I put it inexamples orusers_explain?

@tacaswell
Copy link
Member

  1. Should I try to fix this behavior or just write into the docs that it shouldn't be used like that?

Please try to fix thisor make it error in this case

  1. there should be a way to do this without calling.gca()
  2. that seems reasonable
turnipseason reacted with thumbs up emoji

@tacaswell
Copy link
Member

writing output... [  4%] api/_as_gen/matplotlib.animation.AbstractMovieWriter .. api/_as_gen/matplotlib.artist.Artist.set_visible/home/circleci/project/lib/matplotlib/pyplot.py:docstring of matplotlib.pyplot.subfigure_mosaic:6: WARNING: unknown document: '/gallery/subplots_axes_and_figures/subfig_mosaic'writing output... [  8%] api/_as_gen/matplotlib.artist.Artist.set_zorder .. api/_as_gen/matplotlib.axes.Axes.get_axisbelowwriting output... [ 12%] api/_as_gen/matplotlib.axes.Axes.get_box_aspect .. api/_as_gen/matplotlib.axes.Axes.pcolorfastwriting output... [ 16%] api/_as_gen/matplotlib.axes.Axes.pcolormesh .. api/_as_gen/matplotlib.axes.Axes.vlines/home/circleci/project/lib/matplotlib/figure.py:docstring of matplotlib.figure.FigureBase.subfigure_mosaic:6: WARNING: unknown document: '/gallery/subplots_axes_and_figures/subfig_mosaic'/home/circleci/project/lib/matplotlib/figure.py:docstring of matplotlib.figure.FigureBase.subfigure_mosaic:6: WARNING: unknown document: '/gallery/subplots_axes_and_figures/subfig_mosaic'/home/circleci/project/lib/matplotlib/figure.py:docstring of matplotlib.figure.FigureBase.subfigure_mosaic:6: WARNING: unknown document: '/gallery/subplots_axes_and_figures/subfig_mosaic'

This look related to this PR.


This is a helper function to build complex GridSpec layouts visually.

See :doc:`/gallery/subplots_axes_and_figures/subfig_mosaic`
Copy link
Member

Choose a reason for hiding this comment

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

This does not appear to have been added?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I haven't added the example yet. Working on it, though!

# separate SubFigures for nested layouts,
# otherwise they will all create without nesting.
subfigs_for_nested = {}
if sub_add_func.__name__ == 'add_subfigure':
Copy link
Member

Choose a reason for hiding this comment

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

Checking the name of a passed in method is generally a bad sign!

To understand the problem here it is that when there are nested sub figures we need to switch which (sub) figuresadd_subfigure we pass down through?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Essentially, yes. I wanted to keep track of all of the nested figures (in any 'level' of nesting) with thesubfigs_for_nested dictionary, so that later on I could assign the correct nested figures their parent 'dummy' figure (and they got positioned correctly). Sincesubplot_mosaic worked just fine with nesting already, I did this.

@@ -2084,7 +2062,9 @@ def _do_layout(gs, mosaic, unique_ids, nested):
nested_output = _do_layout(
gs[j, k].subgridspec(rows, cols),
nested_mosaic,
*_identify_keys_and_nested(nested_mosaic)
*_identify_keys_and_nested(nested_mosaic),
sub_add_func,
Copy link
Member

Choose a reason for hiding this comment

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

If you did this as

subadd_func if subfigs_for_nested.get(key) is None else subfigs_for_nested[key].add_subfigure:

then you would still only need to pass one piece of information into the recursive call

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ok, I think I see what you mean. I changed it to what you suggested, plus the internal code for the_get_subfigs_from_nested and it seems to work. I'll test it a bit more and amend the PR if it looks good. Thank you!


A visual layout of how you want your Axes to be arranged
labeled as strings. For example ::
sub_add_func : `.Figure.add_subplot` for `.Figure.subplot_mosaic` and
Copy link
Member

@tacaswelltacaswellJul 20, 2023
edited
Loading

Choose a reason for hiding this comment

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

ok, so I think the fix to this nested figure problem is to change the signaturesub_add_func. The input should stay the same, but the return should be the returned thingand the nextsub_add_func to use.

For axes it would be

def_add_func(*args,**kwargs):returnself.add_subplot(*args,**kwargs),_add_funcreturnself._sub_prec(mosaic,_add_func, ...)

and for subfigures it would be a bit fancier like

def_factory(fig):def_add_sub_func(*arg,**kwargs):new_fig=fig.add_subfigure(*args,**kwargs)returnnew_fig,_factory(new_fig)returnself._sub_prec(mosaic,_factory(self), ...)

and then where you call it it would be

new_thing, sub_add_func = sub_add_func(....)

That way we do not have to special case anything in the_sub_prep code or have the awkward sometimes we use one input sometimes we use the other behavior on the internal API.

Copy link
Member

Choose a reason for hiding this comment

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

I also have not tried this code so it is probably only 90% correct, but I am pretty confident it is pointing in the right direction.

turnipseason reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Just to not leave this unanswered after my previous comments elsewhere - this will definitely require some thought. Sorry I can't do it right away, but I'll get on it in the morning and, hopefully, make it work :)

Copy link
Member

Choose a reason for hiding this comment

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

"recursion" is called that because you curse it again and again.....

sf.set_linewidth(2.1)
identify_subfigs(subfigs)

# %%
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The subplot_mosaicexample also has a version of this using gridspec_kw. But for subfigures the following analogous code:

mosaic="""AA            BC"""fig=plt.figure()subfigs=fig.subfigure_mosaic(mosaic,subfigure_kw={'edgecolor':'black','linewidth':2},gridspec_kw={"bottom":0.25,"top":0.95,"left":0.1,"right":0.5,"wspace":0.5,"hspace":0.5,    },)identify_subfigs(subfigs)subfigs=fig.subfigure_mosaic(mosaic,subfigure_kw={'edgecolor':'black','linewidth':2},gridspec_kw={"bottom":0.05,"top":0.75,"left":0.5,"right":1,"wspace":0.5,"hspace":0.5,    },)identify_subfigs(subfigs)

gives this:

when it should look like this:

So the only gridspec_kws that are actually being taken into account (if specified) are width/height ratios.

@tacaswell
Copy link
Member

I took the liberty of rebasing this again + a few little fix ups at it seems to work.

@tacaswell
Copy link
Member

I thought I was going to needget_figure(root=False) but it turns out I did not so dropped that commit.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
Status: Waiting for author
Milestone
future releases
Development

Successfully merging this pull request may close these issues.

[ENH]: subfigure_mosaic
5 participants
@turnipseason@rcomer@tacaswell@ksunden@melissawm

[8]ページ先頭

©2009-2025 Movatter.jp