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: remove redundant _make_twin_axes from _subplots#21413
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
OK, so now this fails if you try an apply One method is to make this axes a child of the parent, and then it won't appear in the axes list for the figure. That, however, comes with z-order changes. Currently ax2 is drawnafter the parent, because it was added after. If we make it a child it can be drawn before some artists in the parent. |
Interestingly, I think that is sometimes something people want? At least when using pickers and stuff. But it would likely be a breaking change to modify the order. |
For sure, I think people want more control over where the twin is relative to the parent. However, it may also break a lot of people, and I'm not clear how we deprecate it in an obvious way. |
Perhaps a solution would be to make tight_layout autoignore any axes that have an axes_locator set? (after all those probably have their own idea of "where they want to go".) Or a safer approach would be to add another flag "ignored_by_layout_engine" and set that flag explicitly for twins (perhaps this can be made private, as this should mostly only affect twins, which are not child axes due to historical causality). |
Well, that doesn't work because we allow the contents of the twin to be included in the extent of the parent. It would be nice to just make the twin a child of the parent, but again that has draw order implications. I think that this new version works ok - it just adds an extra check and allows the child twin to take its parent subplot spec. I feel that twins were somewhat questionably defined as peers at some point, whereas life would be easier if one was the parent and the other the child. |
lib/matplotlib/_tight_layout.py Outdated
| if axes_or_locator is None: | ||
| axes_or_locator = ax | ||
| elif hasattr(ax, '_twinned_axes'): |
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.
The hasattr check is unnecessary, as it's a class variable.
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.
Oh I see. Well, this fix here is therefore not very good logic. Making the twinned axes siblings is problematic for the original issue - if we move one of the twins, the other twin is supposed to come along. But that implies one of the twins is the parent and one the child.
I guess the redundant_make_twin_axes should remain for the two cases, and likely what needs to change isset_postiion to also change the position of any twins....
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.
if we move one of the twins, the other twin is supposed to come along
But does it even make sense to call set_position on an axes that has an axes_locator set? both are basically trying to force the axes' position. Perhaps e.g. set_position could check if an axes_locator exists and ask the user to explicitly callset_axes_locator(None) first if they really want the axes to go here and nowhere else? In that case, for twin axes, the correct way to set their position would be to set the "parent" axes' position, not the child one (which seems correct?) (Yes, I know, it's not a "parent-child" relationship in the usual sense.)
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.
Sure, OK, lets try this. If an axes has a locator, but it is a twin, see if the twin has a locator; if not, and it has a subplotspec, use the subplotspec for tight layout. This all seems a little more convoluted than it should be, but....
@jklymak I stumbled upon the idea of turning twin axes into child axes as well, and likewise ran into the draw-ordering problem. However, I think this can be solved just be setting a high enough zorder on the twin, so that it gets drawn last? (This means one can break the zordering by inserting artists with custom zorders even higher, but I think we can live with that, or if we want to be super careful that's something we can explicitly detect and warn about during the transition period.) |
...or the twin could be a special child of the parent axes that always gets completely drawn either before or after the parent. That would require another list and a bit of logic to determine if it should be drawn first or second (currently I believe it's drawn second). The original issue was that if the parent was moved and it was created with |
To be clear, (unless I got that wrong...) currently child axes are completely drawn as a single block within the parent axes children, i.e. if there's parent axes with
then the draw order is ABCD, not BADC: the zorder of B and C are only used within the child axes and are effectively in a different scale from the zorders of A, X, and D (I think). So to achieve the desired effect we just need to make the zorder of the child-shared axes very large and all its subartists will be drawn after all (other) subartists of the parent. |
For sure I agree, zorder is one way to make the child behave. OTOH, I'm suggesting maybe we want to have child axes or some twin axes in a different list just to remove the ambiguity with normal Artist zorder. |
I'm not sure that really helps here? (and anyways that could be added later I guess) |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Closes#21409
For some reason we defined
_make_twin_axestwice, and the_subplotsversion doesn't track movement of the original axes.EDIT:
OK, the
_base_make_twin_axesused an axes_locator to position the twin. The_subplotsversion just gave the twin asubplotspecand used that for positioning.In#21409, the user was doing
and the twin
ax2was put in the subplotspec position rather than the new position of the parent.The change here is to always have the twin located by the parent, regardless of how the parent was created. I think that is correct, but I guess its debatable whether we want to change this, or if we want to just tell the user to create the original axes without the subplotspec mechanism. (ie. w/
fig.add_axes([0.5, 0.5, 1, 1]))In either case, this isn't urgent for 3.5.2, so I've moved to 3.6
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).