Do not pass gridspec_kw to inner layouts in subplot_mosaic #24189
Conversation
Co-authored-by: Jody Klymak <jklymak@gmail.com>
|
Sorry Sorry, should have been |
|
@jklymak I've updated those lines to |
|
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 supplied
I do not agree with this and this were a case, we should rip out all of the recursive logic in |
That was discussed above - what is left are
Almost all the subplot mosaic calls I've seen are made with 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. |
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 deprecate @joshbarrass Sorry for over-reacting a bit on your first PR, I assure you it is not personal. |
I did not read enough context and over-reacted. Would still like the deprecation message to be a worded a bit more strongly, but will not block over that.
|
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? |
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
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! |
…b#24189) * Do not pass width/height ratios to nested layouts Co-authored-by: Jody Klymak <jklymak@gmail.com> Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
PR Summary
Constructs a newgridspec_kwdictionary with the"width_ratios"and"height_ratios"removed, which is passed to the inner layouts instead of the original. This prevents an error from occurring when one of the inner layouts is not compatible with one of the specified ratios.The
gridspec_kwarg when constructing a nested layout withsubplot_mosaicis no longer passed to any of the inner layouts. These args are generally not applicable to any of the inner layouts, and can cause exceptions to be raised in some cases (for example, the"width_ratios"and"height_ratios"cause an error when one of the nested layouts is not compatible with one of these ratios). More complex layouts should be constructed directly using.subfigureand.subplots.Fixes #24099
Resubmission of #24188 with a more appropriate branch name.
Currently has one warning when building the docs, which will be fixed by #24190PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).