Repository navigation
ENH: accept 'all'/'row'/'col' axis sharing in subplot_mosaic - #32239
aimldlds wants to merge 1 commit into
Conversation
Sorry, something went wrong.
Figure.subplot_mosaic only accepted booleans for sharex and sharey, while Figure.subplots also takes 'none', 'all', 'row' and 'col'. Accept the string spellings in the mosaic API too, so the two APIs agree. A mosaic Axes may span several rows or columns, so it is attributed to every row or column it covers and spanning Axes transitively merge the groups they bridge. Grouping uses cbook.Grouper, and nested mosaics are resolved to their outer cell via SubplotSpec.get_topmost_subplotspec, so nested layouts are handled rather than rejected. Refs matplotlib#18305.
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
Sorry, something went wrong.
|
Hi @aimldlds
Could you say something about how you settled on this rule? Do you have a specific use case in mind? |
Sorry, something went wrong.
|
Without having thought too much about this: I would have intuitively settled on a different rule: Axes with the same span form a group. So for sharey="row" and ABCD EFCD the groups are AB, CD, EF. The motivation is that shared axes are mostly used to make values in different axes visually comparable by aligning data values. If the sizes of the Axes differ, e.g. between B and C, that visual alignment of data values is lost and thus there is little reason to lock the ranges. |
Sorry, something went wrong.
|
Yes that would be my expectation as well. |
Sorry, something went wrong.
|
I think grouping on matching span would also match the suggestion in the OP of #18305, as well as #26327 (review). Since @aimldlds stated he directed the choices I thought maybe he had a reason for doing something different. But I misread: he also stated that Claude Code proposed the rule. @aimldlds please have a look at our AI policy. It exists to preserve maintainers’ time. The human author must take full responsibility for the change, and that includes reading and understanding what has already been written about the problem. Thank you for your honesty about how you used AI. I am going to close this here because I think you are relying too much on the AI output rather than understanding the problem for yourself, and those circumstances often do not lead to productive reviews. You are welcome to open future PRs taking the AI policy into account. |
Sorry, something went wrong.
PR summary
subplot_mosaic only took True/False for sharex/sharey, but subplots also takes 'none'/'all'/'row'/'col'. PR #20107 added the booleans in 2021 and left the strings for later. This adds them.
The open question is what 'row'/'col' mean when an Axes spans several cells. The rule here: an Axes belongs to every row/column it covers, so spanning Axes transitively merge the groups they bridge. In 'AAE;C.E', where E spans both rows, sharex='row' groups A, C and E together, while sharex='col' gives {A, C} and {E}.
Grouping uses cbook.Grouper (the union-find already backing _shared_axes), and nested mosaics resolve to their outer cell via SubplotSpec.get_topmost_subplotspec, so nested layouts are handled rather than rejected.
I would value maintainer feedback on the spanning-Axes semantics - it is the reason this part of #18305 stayed open, and I am happy to change the rule.
Refs #18305.
AI Disclosure
AI (Claude) was used substantially in this PR: it explored the codebase, proposed the spanning-Axes rule, and drafted the implementation, the tests and the documentation. I reviewed the result and directed the choices, but I did not write the code myself.
Verification so far: the 40 existing TestSubplotMosaic tests plus the 9 added here pass, and ruff is clean - but that run was against a released matplotlib binary with this patch applied, not against a local build of this branch. I have not yet built matplotlib locally, so CI here is the first full test of the actual tree.
I am aware of the project's policy on AI use in good-first-issues. I am disclosing fully so maintainers can judge whether this is useful to them, and I will accept the outcome either way.
PR quality check
Use an expressive title, e.g. "Fix title font property precedence"
New and changed code is tested (9 new tests; see caveat in AI Disclosure)
N/A Plotting related features are demonstrated in an example
New features and API changes have release notes
Documentation complies with general and docstring guidelines