Skip to content
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

Doc: use empty ScalarMappable for colorbars with no associated image. #16037

Merged
merged 2 commits into from
Dec 31, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Dec 29, 2019

This is consistent with the docstring of colorbar(), which explicitly
suggests this approach; this would also later allow deprecating
ColorbarBase, merging it into Colorbar.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

This is consistent with the docstring of colorbar(), which explicitly
suggests this approach; this would also later allow deprecating
ColorbarBase, merging it into Colorbar.
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this, but won't merge just yet in case anyone was strongly wedded to ColorbarBase for some reason.

@anntzer
Copy link
Contributor Author

anntzer commented Dec 30, 2019

(note that this was (somewhat) discussed in #3644)

@anntzer
Copy link
Contributor Author

anntzer commented Dec 30, 2019

Added a second commit which replaces cb = fig.colorbar(...); cb.set_label(label) by fig.colorbar(..., label=label) because that seems simpler too.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a usability point of view, creating a colorbar object directly seems simpler. Then again, we usually use factory functions and rarely instantiate objects directly in matplotlib.

From a design perspective, I'm not quite clear who owns the cmap and norm.

  • If colormaps would not hold cmap and norm attributes but just query the associated mappable, the approach of this PR makes sense.
  • Looking at the code it seems that ColorbarBase actually manages it's own cmap and norm. Colorbar again copies these attributes from the mappable and tries to keep in sync via on_mappable_changed.

In the end, I'm +0.5 because it's already a documented recommendation and it paves the way for deprecating ColorbarBase, and isolated colorbars are a more exotic feature, which is ok to be a bit more cumbersome.

@timhoffm timhoffm added this to the v3.2.0 milestone Dec 31, 2019
@timhoffm timhoffm merged commit 9c2ad59 into matplotlib:master Dec 31, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Dec 31, 2019
timhoffm added a commit that referenced this pull request Dec 31, 2019
…037-on-v3.2.x

Backport PR #16037 on branch v3.2.x (Doc: use empty ScalarMappable for colorbars with no associated image.)
@anntzer anntzer deleted the colorbarbaseless branch December 31, 2019 21:34
@anntzer
Copy link
Contributor Author

anntzer commented Dec 31, 2019

My plan is to make Colorbar.cmap and Colorbar.norm just properties returning the underlying mappable's cmap and norm, but that'll happen in the big ColorbarBase-deprecating PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants