Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix label_outer in the presence of colorbars.#30098
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| return | ||
| gs = ss.get_gridspec() | ||
| if (isinstance(gs, mpl.gridspec.GridSpecFromSubplotSpec) | ||
| and gs.nrows * gs.ncols == 6): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please add a comment what this magic is about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks. Basically ok.
Would it make more sense thatmake_axes_gridspec explicitly flags the gridspec? Not sure what the right level of explicitness / specialization is, but it feels awkward to detect a gridspec for the colorbar thrugh nrows * ncols. Options could be (from less to more formal):
- monkey-patch
_is_colorbar_gridspeconto the gridspec, so that we can testif hasattr(gs, "_is_colorbar_gridspec") - add a
_is_colorbar_gridspecattribute to all gridspecs, so that we can testif gs._is_colorbar_gridspec - Create a
ColorbarGridSpecsubclass so that we can testif isinstance(gs, ColorbarGridSpec)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would lean towards the least formal here - the subgridspec trick of making colorbars is pretty clunky, and note that it is explicitlynot used bylayout='constrained'.
The original ideas behind grid specs and subgridspecs were very good, but unfortunately loses the relationship between sibling axes if you change add the colorbar and make the colorbar and main axes child subgridspecs.
The subgridspec to be considered should be the one containing both theaxes and the colorbar, not the sub-subgridspec of just the axes.
| f, axs = plt.subplots(2, 2, sharex=True, sharey=True) | ||
| @pytest.mark.parametrize('with_colorbar', [True, False]) | ||
| def test_label_outer(remove_ticks, with_colorbar): | ||
| fig, axs = plt.subplots(2, 2, sharex=True, sharey=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can this get tested with layout=constrained as well?
The subgridspec to be considered should be the one containing both the axes and the colorbar, not the sub-subgridspec of just the axes.
Closes#27305 (using#27305 (comment) and a test).
PR summary
PR checklist