Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix: bottom and left text appearance in colorbar #25438

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xtanion wants to merge 3 commits into matplotlib:main from xtanion:cb_axes_fix

Conversation

Copy link
Contributor

@xtanion xtanion commented Mar 12, 2023
edited
Loading

PR Summary

This PR fixes CbarAxes.toggle_label inconsistency as mentioned in issue #23595
closes #23595

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

Copy link
Member

It is a bit hard to say what it actually should do as there is no doc-string, but I am not sure this is the right fix.

  1. Passing toggle_label(True) should switch them back on.
  2. This only covers half of the possible cases.

(It is a bit strange that orientation is actually what one would think is location.)

I think a better solution is to make a dict that picks the "opposite" axis name and then uses that. (If that is now the solution.)

xtanion reacted with thumbs up emoji

Copy link
Member

Looking at https://matplotlib.org/3.3.4/gallery/axes_grid1/demo_axes_grid.html#sphx-glr-gallery-axes-grid1-demo-axes-grid-py (which probably shows the "correct" behaviour), it seems that maybe the solution is to put the ticks and the labels on the top when "orientation" is "top" etc. In this way toggle_label will work as expected.

xtanion reacted with thumbs up emoji

Copy link
Contributor Author

xtanion commented Mar 14, 2023
edited
Loading

Hi @oscargus, I updated the code as you suggested, and It looks like the correct result now :
image

edit: Should the colorbar show ticks when set_labels is set to False. It is visible in this example, But this PR currently removes the ticks as well is set_labels is False.

Copy link
Member

edit: Should the colorbar show ticks when set_labels is set to False. It is visible in this example, But this PR currently removes the ticks as well is set_labels is False.

If there are no tick labels, ticks don't have a meaning. So, whether or not to remove them is a purely aesthetic decision. IMHO it's fine to remove them.

xtanion reacted with thumbs up emoji

@xtanion xtanion changed the title (削除) fixes bottom and left text appearance (削除ここまで) (追記) Fix: bottom and left text appearance in colorbar (追記ここまで) Mar 24, 2023
Copy link
Contributor Author

xtanion commented Mar 25, 2023

If there are no tick labels, ticks don't have a meaning. So, whether or not to remove them is a purely aesthetic decision. IMHO it's fine to remove them.

Thanks for confirming @timhoffm, Is there any additional changes needed?

Copy link
Member

Gentle ping @oscargus - would you mind taking a look again?

Copy link
Member

rcomer commented Apr 22, 2023

Hi @xtanion, thank you for your work on this and sorry it has taken a while to get back to you.

It’s great that you have confirmed locally that the method now has the correct behaviour. Could you simplify the code you checked with and make it an image comparison test? Simplify because it just needs enough to prove the desired behaviour is happening with the ticks (so e.g. no need for the annotations).

The documentation build check has failed due to a problem that I think has been fixed on main. So could you also rebase your branch to pick up that fix? That would allow us to see how your change affects the original example from the issue.

xtanion reacted with thumbs up emoji

Copy link
Contributor

anntzer commented May 13, 2023
edited
Loading

Will that work if the colorbars are on the bottom or the left side? I'm not so sure...

Edit: I think this is not the right approach at all, see #25884 instead.

Copy link
Contributor

anntzer commented May 15, 2023

Superseded by #25884. Thanks for the PR!

xtanion reacted with thumbs up emoji

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: CbarAxesBase.toggle_label doesn't seem to work properly

AltStyle によって変換されたページ (->オリジナル) /