FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[WIP] axes_grid1: make twin* not modify host axes by smheidrich · Pull Request #13092 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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

Draft
smheidrich wants to merge 1 commit into
matplotlib:mainfrom
smheidrich:twins_should_not_change_host
ConversationCommits (1)ChecksFiles changed
Draft

[WIP] axes_grid1: make twin* not modify host axes#13092
smheidrich wants to merge 1 commit into
matplotlib:mainfrom
smheidrich:twins_should_not_change_host

Conversation

smheidrich commented Jan 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Post-PR behavior

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 commented Jan 3, 2019

Copy link
Copy Markdown
Member

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 commented Jan 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 commented Jan 3, 2019

Copy link
Copy Markdown
Member

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 commented Jan 3, 2019

Copy link
Copy Markdown
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):

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

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

jklymak commented Jan 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Jan 3, 2019

Copy link
Copy Markdown
Contributor

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 force-pushed the twins_should_not_change_host branch 2 times, most recently from 2d25e62 to e499b2f Compare January 3, 2019 23:52
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 force-pushed the twins_should_not_change_host branch from e499b2f to 23a3b6e Compare January 4, 2019 00:34

jklymak commented Jan 4, 2019

Copy link
Copy Markdown
Member

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 commented Feb 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 mentioned this pull request Oct 17, 2019
Merged
6 tasks
jklymak marked this pull request as draft July 23, 2020 16:44

jklymak commented Jul 23, 2020

Copy link
Copy Markdown
Member

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

smheidrich commented Jul 27, 2020

Copy link
Copy Markdown
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 commented Aug 4, 2020

Copy link
Copy Markdown
Member

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 mentioned this pull request Sep 15, 2020
Open

github-actions Bot commented May 27, 2023

Copy link
Copy Markdown

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 Bot added the status: inactive Marked by the “Stale” Github Action label May 27, 2023

This branch has not been deployed

No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL