When removing colorbar, restore the correct parent axes. #31040
Conversation
| np.testing.assert_allclose(pre_position.get_points(), | ||
| post_position.get_points()) | ||
| np.testing.assert_allclose( | ||
| pre_position.get_points(), post_position.get_points(), atol=0.0002) |
There was a problem hiding this comment.
Do both orphan_mappable values need the tolerance or is this specific to the new orphan_mappable = True case?
There was a problem hiding this comment.
Good catch. Actually this is more complicated than expected, because constrained_layout uses make_axes, not make_axes_gridspec, i.e. it needs another position restoring strategy. I'll need to check whether all the relevant info is in colorbar_info...
There was a problem hiding this comment.
I think if you remove the colorbar under constrained layout it should just readjust on its own? My memory is hazy, but I'm pretty sure there is a "column" (or row) reserved for the colorbar, and if there is no colorbar it just collapses to zero width (height)?
lib/matplotlib/colorbar.py
Outdated
| for a in parents: | ||
| if self.ax in a._colorbars: | ||
| a._colorbars.remove(self.ax) |
There was a problem hiding this comment.
While at it, let's make the naming more clear
| for a in parents: | |
| if self.ax in a._colorbars: | |
| a._colorbars.remove(self.ax) | |
| for ax in parents: | |
| if self.ax in ax._colorbars: | |
| ax._colorbars.remove(self.ax) |
Also, semi-OT and probably rather suitable for a separate PR:
- I think parents is somewhat a misnomer. Unlike e.g.
inset_axes()the colorbar Axes is not added as a child to the Axes it steals from, i.e.self.ax in self._axes_info["parents"][0].get_children()is False. getattr(self.ax, "_colorbar_info", {}).get("parents", [])feels to dynamic. Should we (1) turn the dict into a dataclass (fixed set of defined fields)?
There was a problem hiding this comment.
- Renamed to ax.
- The second get() was only to handle the case where _colorbar_info doesn't exist at all (if self.ax was not created by make_axes -- typically a manually created axes passed as
colorbar(cax=...)). I made this a bit simpler. - I'll punt renaming "parents" for another time...
| and parents[0].get_subplotspec() | ||
| and (self.ax.get_subplotspec().get_gridspec() | ||
| is parents[0].get_subplotspec().get_gridspec())): | ||
| parents[0].set_subplotspec( |
There was a problem hiding this comment.
Why do we only reset parents[0]. What happens for the case colorbar(ax=[ax1, ax2])?
There was a problem hiding this comment.
If borrowing from multiple colorbars we never actually rely on use_gridspec=True (see Figure.colorbar) so self.ax.get_subplotspec() will already be false above; in any case I also check for len(parents) == 1 just above just to be safe.
To figure out which axes need to be restored, `mappable.axes` is less reliable than `_colorbar_info["parents"]`. Why the tests now need a greater (though still tiny) tolerance to make the "orphan mappable" case pass is a mystery.
To figure out which axes need to be restored,
mappable.axesis less reliable than_colorbar_info["parents"].Why the tests now need a greater (though still tiny) tolerance to make the "orphan mappable" case pass is a mystery.
Closes #22257.
PR summary
PR checklist