Sorry

home/circleci/project/lib/matplotlib/figure.py:docstring of matplotlib.figure.Figure:49: WARNING: py:obj reference target not found: subfigure/home/circleci/project/lib/matplotlib/figure.py:docstring of matplotlib.figure.Figure:49: WARNING: py:obj reference target not found: subfigure/home/circleci/project/lib/matplotlib/figure.py:docstring of matplotlib.figure.Figure:49: WARNING: py:obj reference target not found: subfigure/home/circleci/project/doc/api/next_api_changes/behavior/24189-JB.rst:4: WARNING: py:obj reference target not found: Figure.subfigure

Sorry, should have been.Figure.subfigures I think...

@jklymak I've updated those lines to.Figure.subfigures now. Fingers crossed everything works now (:

Are there any uses of passing things through that will now be broken?

I do not think this in the right fix as the issue here is that the height and width ratios are being passed through not generic gridspec kwargs. I think instead we should not merge the two ratio into the user supplied dictionary and only use those at the top level and let the user suppliedgridspec_kw dictionary still flow all the way through as there are other keywords that you would want to thread through that do not depend on exactly matching the shape of the layout.

More complex layouts should be constructed directly using .subfigure and .subplots.

I do not agree with this and this were a case, we should rip out all of the recursive logic insubplot_mosaic.

as there are other keywords that you would want to thread through that do not depend on exactly matching the shape of the layout.

That was discussed above - what is left arew/hspace,left/top etc. Is there really a case where a user has set these, and also expects the same values to be passed down to lower levels? These are all passed in figure-relative units, so they will all halve in physical size at lower levels. It would be a pretty fortuitous layout that wanted this.

More complex layouts should be constructed directly using .subfigure and .subplots.

I do not agree with this and this were a case, we should rip out all of the recursive logic in subplot_mosaic.

Almost all the subplot mosaic calls I've seen are made withlayout='constrained' which ignoresw/hspace,left/top etc. anyhow. So unless we want to allow the user to have a nested set of gridspec_kws I think the current proposal is the cleanest.

BTW, the above should read: More complex layouts should be constructed directly using .subfigure and .subplots or .subplot_mosaic. You can still use subplot_mosaic on the subfigure.

That was discussed above - what is left are w/hspace, left/top

Sorry, I did not unfold and read the in-line discussion and was just reacting to the current state of the PR. I now see that there was extensive discussion in them.

I find the argument that none of these arguments make sense recursively (and passing them through at all was a mistake (that is my fault)). I think this should be made clearer in the deprecation note as now it reads (to me) like "This works most of the time but we are going to break it because it does not work in some corner cases".

I'm of bit of a mind to deprecategridspec_kw all together I think that is worse (it breaks the rhyming withfig.subplots).

@joshbarrass Sorry for over-reacting a bit on your first PR, I assure you it is not personal.

No problem@tacaswell. I can totally understand where you are coming from, particularly as I originally drafted this PR that way (:

I've updated the message to reflect the discussion, that the behaviour is being changed because it is rarely appropriate for these arguments to be passed to the inner layouts. Does this make the reasoning for the change clearer in everyone's opinions?

I squashed merged this to keep the history a bit cleaner.

Thank you for you work@joshbarrass and congratulations on your first merged Matplotlib PR 🎉 Hopefully we will hear from you again!