Skip to content

[WIP] axes_grid1: make twin* not modify host axes #13092

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[WIP] axes_grid1: make twin* not modify host axes #13092

wants to merge 1 commit into from

Conversation

smheidrich
Copy link
Contributor

@smheidrich smheidrich commented Jan 3, 2019 •
edited
Loading

PR Summary

PR #4898 changed the behavior of twinx() and twiny() in axes_grid1 to make them consistent with twin(). This commit inverts the "direction" of consistency and makes twin() behave like the pre-#4898 twinx() and twiny() instead. This way, the API becomes less surprising: instead of hiding certain host axes, twin* now keeps the host axes unmodified, while the parasite axes have their axis lines hidden instead (cf. images below).

Unfortunately, this change breaks backwards-compatibility for code relying on the specifics of host and parasite axes visibility.

Helps with #10748.

Relevant image comparison test

The effect of this PR is best illustrated by comparing the results of the test_axes_grid1.test_twin_axes_empty_and_removed image comparison test:

Pre-PR behavior

twin_axes_empty_and_removed-expected-pre20180103

Post-PR behavior

twin_axes_empty_and_removed

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
    • yes according to Travis
  • New features are documented, with examples if plot related
    • not applicable
  • Documentation is sphinx and numpydoc compliant
    • not applicable (nothing about the documentation changed)
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
    • not applicable
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way
    • I put it in doc/api/next_api_changes, because it looks like this is the way it's intended now?
  • (not part of the official checklist but should be done for mpl_toolkits changes): make absolutely certain the gallery examples all still work

@jklymak
Copy link
Member

jklymak commented Jan 3, 2019

How did you make the plots above? I'm not sure I follow all the different options, and by "Old/New" you mean before/after this PR? Because I'm not convinced the "New" behaviour is very good.

I'm not sure we should expect anything sensible to happen to the twin if the parent is removed.

Note that we are hopefully going to have a new way to add "secondary axes" which I think is a more straightfroward paradigm than twin axes... #11859

@smheidrich
Copy link
Contributor Author

smheidrich commented Jan 3, 2019 •
edited
Loading

@jklymak Sorry, I should've specified that the images come from the test_axes_grid1.test_twin_axes_empty_and_removed test (I've added this to the PR description now).

I'm not sure we should expect anything sensible to happen to the twin if the parent is removed.

I'm not sure what you mean by that; parent (i.e. host) axes removal isn't part of the test and I think is also unchanged by this PR. This PR only changes how the various twin* axes are constructed (i.e. whether axis lines are re-used from the host axes or not) and in the course of that simplifies what happens when the twin axes are removed. Before this PR, twin* would make the host axes' axislines corresponding to the twin axes invisible, so that the twin's axislines are drawn instead. This PR reverses that: the twin axislines are made invisible, and the host axislines are "re-used". The people in #10748 agreed that this behavior is less surprising than the previous one.

Note that we are hopefully going to have a new way to add "secondary axes" which I think is a more straightfroward paradigm than twin axes... #11859

That sounds great! I'll have to look into it.

@jklymak
Copy link
Member

jklymak commented Jan 3, 2019

OK, thanks - sorry missed that this is axes_grid1.

I don't see why the spine would be made invisible on either parent or twin at all, but no doubt I'm not following it closely enough. That is a pretty long issue chain. Is it that over-plotting the spine looks bad in some way?

@smheidrich
Copy link
Contributor Author

@jklymak I think the decision to have only one axisline goes back even further than #4898 let alone this PR here; all #4898 did was to make the decision which one gets hidden consistent between twinx/twiny and twin (you can see the old, inconsistent behavior in the top image here).

Regarding drawing both axislines on top of each other, here is what that would look like (2nd row):

twin_axes_empty_and_removed

In my opinion, it is noticeable and does look a bit bad.

@smheidrich smheidrich force-pushed the twins_should_not_change_host branch from 8f793f7 to a54f4c7 Compare January 3, 2019 21:13
@jklymak
Copy link
Member

jklymak commented Jan 3, 2019 •
edited
Loading

It is noticeable, but I wonder why? Aren't they both lines drawn at the same value of y? Is it bad in all backends, and does it get less bad with higher dpi? I wonder in @anntzer mpl_cairo looks bad...

EDIT: looks like the anti-aliasing kicking in twice? i.e. the shading is darker around the full-black part of the line?

@anntzer
Copy link
Contributor

anntzer commented Jan 3, 2019

Overdrawing a line twice will always look not-so-great in the presence of antialiasing; pretty sure mplcairo would give you the same results (not that I tried).

@smheidrich smheidrich force-pushed the twins_should_not_change_host branch 2 times, most recently from 2d25e62 to e499b2f Compare January 3, 2019 23:52
@smheidrich
axes_grid1: make twin* not modify host axes
23a3b6e 
PR matplotlib#4898 changed the behavior of twinx() and twiny() in axes_grid1 to
make them consistent with twin(). This commit inverts the "direction" of
consistency and makes twin() behave like the pre-matplotlib#4898 twinx() and
twiny() instead. This way, the API becomes less surprising: instead of
hiding certain host axes, twin* now keeps the host axes unmodified,
while the parasite axes have their axis lines hidden instead.

Unfortunately, this change breaks backwards-compatibility for code
relying on the specifics of host and parasite axes visibility.

Helps with matplotlib#10748.
@smheidrich smheidrich force-pushed the twins_should_not_change_host branch from e499b2f to 23a3b6e Compare January 4, 2019 00:34
@jklymak
Copy link
Member

jklymak commented Jan 4, 2019

OK, so given this, I agree that it makes sense to have the twin be the one that doesn't draw its spine.

I don't know the twinx behaviour well enough, but is the axis always over top of the parent spine? If not, then does it know if its displaced so it can make the spine visible?

Otherwise I think this change of behaviour is good.

@smheidrich
Copy link
Contributor Author

smheidrich commented Feb 5, 2019 •
edited
Loading

Sorry for the long inactivity.

I don't know the twinx behaviour well enough, but is the axis always over top of the parent spine? If not, then does it know if its displaced so it can make the spine visible?

The only example of displaced parasite axes I know of is the demo_parasite_axes2 gallery example, in which the shifted axis has to be made visible manually. This line seems to have been introduced at least 9 years ago, so I guess it has always been like this.

The line directly above it had to be introduced after #4898 broke the example, and could in principle be removed again given the changes in this PR (although it doesn't do harm to leave it in, either).

@anntzer anntzer mentioned this pull request Oct 17, 2019
Merged
6 tasks
@jklymak jklymak marked this pull request as draft July 23, 2020 16:44
@jklymak
Copy link
Member

jklymak commented Jul 23, 2020

@smheidrich This didn't get any follow up, but still seems useful. Feel free to revive!

@smheidrich
Copy link
Contributor Author

I think all that was left to do here was check whether all the gallery and doc examples are definitely unaffected by the changes in this PR (because the original PR that this one partially corrects messed something up). They're still not part of the baseline-comparison CI tests, are they?

@QuLogic
Copy link
Member

QuLogic commented Aug 4, 2020

No, but if there's something that fails there, it should probably be added to the tests. However, docs (including most examples) are built as part of CI.

@anntzer anntzer mentioned this pull request Sep 15, 2020
Open
@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: inactive Marked by the “Stale” Github Action status: needs rebase topic: mpl_toolkit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@smheidrich @jklymak @anntzer @QuLogic