Adds row and column axes sharing to simple subplot mosaics #26327
Conversation
There was a problem hiding this comment.
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.
You can also join us on gitter 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 I used my forked main branch. Should I rename the branch then recreate the PR? |
|
@ketozhang that would be ideal, if you can do that. Thanks! |
There was a problem hiding this comment.
I think leaving out nested specs is fair (I could make some arguments as to how they might work, but even so that gets confusing pretty quickly)
I think a test which includes axes which span more than one gridspec cell would be good. (not nested, but something like
ABBC
ADDC
EFGH
Where I would expect, I think, A/E B/D and C/H to share x axes as pairs (given sharex="col") and A/C E/F/G/H to share y axes (given sharey="row"
But specifically B/D should not share y with A/C and F/G should not share X with B/D
As for reopening from non-main branch: It is ideal, but mostly to make it easier for you to have upstream/main readily available (e.g. to make additional PRs)
To do so:
$ git switch -c mosaic_row_col # (just a name, make sure the changes are saved somewhere)
$ git switch main
$ git reset --hard upstream/main # (or origin/main, depending on which name you used for matplotlib/matplotlib)
$ git push --force-with-lease(And open a new PR from your new branch)
You may also wish to consolidate commits if you feel comfortable doing so (if not, we can squash merge on our end).
| # Only accept strict bools to allow a possible future API expansion. | ||
| _api.check_isinstance(bool, sharex=sharex, sharey=sharey) | ||
| _api.check_isinstance((bool, str), sharex=sharex, sharey=sharey) |
There was a problem hiding this comment.
This handling was intended to to disallow "truthy" (but not True, the bool) and "falsy" (but not False, the bool) things from giving the True/False case for sharex/y
Since it is happening after normalization to string (on lines 1943-7) this is not actually doing anything here.
I think that normalization should be moved after this check.
| ax_dict["A"].set(xscale="log", yscale="logit") | ||
| assert all(ax.get_xscale() == "log" and ax.get_yscale() == "logit" | ||
| for ax in ax_dict.values()) | ||
|
|
||
| @check_figures_equal(extensions=["png"]) | ||
| def test_sharex_row(self, fig_test, fig_ref): |
There was a problem hiding this comment.
consider doing the test similar to the test_share_all function above, which sets the scale of shared axes and asserts that all expected sharing is done.
Specifically, without anything actually plotted, there is no way for check_figures_equal (which does an image comparison) to know whether two things are shared or not, as each will default to a range of 0-1 anyway...
There was a problem hiding this comment.
Okay, I will add that. Because sharing axes affect tick labels, I'd like to keep the image comparison.
|
Closing...moved PR to #26411 |
PR summary
Resolves #18305.
Adds row and column axes sharing to simple subplot mosaics. This feature is implemented only for non-nested mosaics. A set of axes are part of the same row/column if it has the same span (axes' start and end row/column are the same).
PR checklist
This is my first code contribution to matplotlib 🎉. Thank you to all folks that helped me at SciPy 2023. I'm sure I missed some things (e.g., how to run formatter), so I appreciate further help on this PR.