-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
introduction of _ColorbarMappable #31039
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -313,7 +313,7 @@ def clip(self, clip): | |||||||||||||||
| self.norm.clip = clip | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| class _ColorizerInterface: | ||||||||||||||||
| class _ColorbarMappable: | ||||||||||||||||
| """ | ||||||||||||||||
| Base class that contains the interface to `Colorizer` objects from | ||||||||||||||||
| a `ColorizingArtist` or `.cm.ScalarMappable`. | ||||||||||||||||
|
Comment on lines
318
to
319
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think the subsequent note needs allow an update. My understanding is that this class is basically "Data+Colorizer" and related functionality like changed. |
||||||||||||||||
|
|
@@ -322,9 +322,94 @@ class _ColorizerInterface: | |||||||||||||||
| attribute. Other functions that as shared between `.ColorizingArtist` | ||||||||||||||||
| and `.cm.ScalarMappable` are not included. | ||||||||||||||||
| """ | ||||||||||||||||
|
|
||||||||||||||||
| def __init__(self, colorizer, **kwargs): | ||||||||||||||||
| """ | ||||||||||||||||
| Base class for objects that can connect to a colorbar. | ||||||||||||||||
|
|
||||||||||||||||
| All classes that can act as a mappable for `.Figure.colorbar` | ||||||||||||||||
| will subclass this class. | ||||||||||||||||
| """ | ||||||||||||||||
| super().__init__(**kwargs) | ||||||||||||||||
| self._colorizer = colorizer | ||||||||||||||||
| self.colorbar = None | ||||||||||||||||
| self._id_colorizer = self._colorizer.callbacks.connect('changed', self.changed) | ||||||||||||||||
| self.callbacks = cbook.CallbackRegistry(signals=["changed"]) | ||||||||||||||||
| self._axes = None | ||||||||||||||||
| self._A = None | ||||||||||||||||
|
|
||||||||||||||||
| @property | ||||||||||||||||
| def axes(self): | ||||||||||||||||
| return self._axes | ||||||||||||||||
|
|
||||||||||||||||
| @axes.setter | ||||||||||||||||
| def axes(self, axes): | ||||||||||||||||
| self._axes = axes | ||||||||||||||||
|
Comment on lines
+341
to
+347
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to be a bit careful with scoping. We've intentionally left out any Artist notion. So this is rather an abstract concept or concern or mixin or trait (or whatever you want to call it). That it needs association with an Axes is not a-priory clear.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good point. Instead of having this here we can have a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could make a very nuanced sematnic distinction here: If you keep the original name OTOH I think I still like the conceptual definition "_ColorMappable = Data + Colorizer" more, even if this means From a quick check, the only usage of matplotlib/lib/matplotlib/colorbar.py Lines 1044 to 1047 in cc1d9b6
So the optional axes may already be cared for? Please re-check. Side-note: I've found matplotlib/lib/matplotlib/colorbar.py Lines 413 to 415 in cc1d9b6
Would it make sense to encode something like can_navigate() into _ColorMappable so that we move the responsibility for this logic from the colorbar to the mappable? - Possibly still needs a better name.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Including @timhoffm @story645 there is a thought I want to run by you.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think every Artist has an axes attribute. Documentation may be currently lacking because it‘s not something a user has to care about and because documenting attributes is a bit more tricky as attributes are just names (not objects like methods) and thus cannot hold a docstring. But it‘s possible - see e.g. #29075. So documentation alone is not enough justification for a property. It’s rather about API. It‘s worth creating a consistent picture on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is likely unrelated, but do figure level artists just have axes set to none?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but it's more complicated behind the scenes. Let's move the topic to #31118. |
||||||||||||||||
|
|
||||||||||||||||
| @property | ||||||||||||||||
| def colorizer(self): | ||||||||||||||||
| return self._colorizer | ||||||||||||||||
|
|
||||||||||||||||
| @colorizer.setter | ||||||||||||||||
| def colorizer(self, cl): | ||||||||||||||||
| _api.check_isinstance(Colorizer, colorizer=cl) | ||||||||||||||||
| self._colorizer.callbacks.disconnect(self._id_colorizer) | ||||||||||||||||
| self._colorizer = cl | ||||||||||||||||
| self._id_colorizer = cl.callbacks.connect('changed', self.changed) | ||||||||||||||||
|
|
||||||||||||||||
| def changed(self): | ||||||||||||||||
| """ | ||||||||||||||||
| Call this whenever the mappable is changed to notify all the | ||||||||||||||||
| callbackSM listeners to the 'changed' signal. | ||||||||||||||||
| """ | ||||||||||||||||
| self.callbacks.process('changed') | ||||||||||||||||
| self.stale = True | ||||||||||||||||
|
|
||||||||||||||||
| def _scale_norm(self, norm, vmin, vmax): | ||||||||||||||||
| self._colorizer._scale_norm(norm, vmin, vmax, self._A) | ||||||||||||||||
|
|
||||||||||||||||
| def set_array(self, A): | ||||||||||||||||
| """ | ||||||||||||||||
| Set the value array from array-like *A*. | ||||||||||||||||
|
|
||||||||||||||||
| Parameters | ||||||||||||||||
| ---------- | ||||||||||||||||
| A : array-like or None | ||||||||||||||||
| The values that are mapped to colors. | ||||||||||||||||
|
|
||||||||||||||||
| The base class `.ScalarMappable` does not make any assumptions on | ||||||||||||||||
| the dimensionality and shape of the value array *A*. | ||||||||||||||||
| """ | ||||||||||||||||
| if A is None: | ||||||||||||||||
| self._A = None | ||||||||||||||||
| return | ||||||||||||||||
|
|
||||||||||||||||
| A = _ensure_multivariate_data(A, self.norm.n_components) | ||||||||||||||||
|
|
||||||||||||||||
| A = cbook.safe_masked_invalid(A, copy=True) | ||||||||||||||||
| if not np.can_cast(A.dtype, float, "same_kind"): | ||||||||||||||||
| if A.dtype.fields is None: | ||||||||||||||||
|
|
||||||||||||||||
| raise TypeError(f"Image data of dtype {A.dtype} cannot be " | ||||||||||||||||
| f"converted to float") | ||||||||||||||||
| else: | ||||||||||||||||
| for key in A.dtype.fields: | ||||||||||||||||
| if not np.can_cast(A[key].dtype, float, "same_kind"): | ||||||||||||||||
| raise TypeError(f"Image data of dtype {A.dtype} cannot be " | ||||||||||||||||
| f"converted to a sequence of floats") | ||||||||||||||||
| self._A = A | ||||||||||||||||
| if not self.norm.scaled(): | ||||||||||||||||
| self._colorizer.autoscale_None(A) | ||||||||||||||||
|
|
||||||||||||||||
| def get_array(self): | ||||||||||||||||
| """ | ||||||||||||||||
| Return the array of values, that are mapped to colors. | ||||||||||||||||
|
|
||||||||||||||||
| The base class `.ScalarMappable` does not make any assumptions on | ||||||||||||||||
| the dimensionality and shape of the array. | ||||||||||||||||
| """ | ||||||||||||||||
| return self._A | ||||||||||||||||
|
|
||||||||||||||||
| def to_rgba(self, x, alpha=None, bytes=False, norm=True): | ||||||||||||||||
| """ | ||||||||||||||||
| Return a normalized RGBA array corresponding to *x*. | ||||||||||||||||
|
|
@@ -514,7 +599,7 @@ def _sig_digits_from_norm(norm, data, n): | |||||||||||||||
| return g_sig_digits | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| class _ScalarMappable(_ColorizerInterface): | ||||||||||||||||
| class _ScalarMappable(_ColorbarMappable): | ||||||||||||||||
| """ | ||||||||||||||||
| A mixin class to map one or multiple sets of scalar data to RGBA. | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -527,15 +612,8 @@ class _ScalarMappable(_ColorizerInterface): | |||||||||||||||
| # and ColorizingArtist classes. | ||||||||||||||||
|
|
||||||||||||||||
| # _ScalarMappable can be depreciated so that ColorizingArtist | ||||||||||||||||
| # inherits directly from _ColorizerInterface. | ||||||||||||||||
| # in this case, the following changes should occur: | ||||||||||||||||
| # __init__() has its functionality moved to ColorizingArtist. | ||||||||||||||||
| # set_array(), get_array(), _get_colorizer() and | ||||||||||||||||
| # _check_exclusionary_keywords() are moved to ColorizingArtist. | ||||||||||||||||
| # changed() can be removed so long as colorbar.Colorbar | ||||||||||||||||
| # is changed to connect to the colorizer instead of the | ||||||||||||||||
| # ScalarMappable/ColorizingArtist, | ||||||||||||||||
| # otherwise changed() can be moved to ColorizingArtist. | ||||||||||||||||
| # inherits directly from _ColorbarMappable. | ||||||||||||||||
| # in this case, all functionality should be moved to ColorizingArtist. | ||||||||||||||||
| def __init__(self, norm=None, cmap=None, *, colorizer=None, **kwargs): | ||||||||||||||||
| """ | ||||||||||||||||
| Parameters | ||||||||||||||||
|
|
@@ -550,63 +628,8 @@ def __init__(self, norm=None, cmap=None, *, colorizer=None, **kwargs): | |||||||||||||||
| cmap : str or `~matplotlib.colors.Colormap` | ||||||||||||||||
| The colormap used to map normalized data values to RGBA colors. | ||||||||||||||||
| """ | ||||||||||||||||
| super().__init__(**kwargs) | ||||||||||||||||
| self._A = None | ||||||||||||||||
| self._colorizer = self._get_colorizer(colorizer=colorizer, norm=norm, cmap=cmap) | ||||||||||||||||
|
|
||||||||||||||||
| self.colorbar = None | ||||||||||||||||
| self._id_colorizer = self._colorizer.callbacks.connect('changed', self.changed) | ||||||||||||||||
| self.callbacks = cbook.CallbackRegistry(signals=["changed"]) | ||||||||||||||||
|
|
||||||||||||||||
| def set_array(self, A): | ||||||||||||||||
| """ | ||||||||||||||||
| Set the value array from array-like *A*. | ||||||||||||||||
|
|
||||||||||||||||
| Parameters | ||||||||||||||||
| ---------- | ||||||||||||||||
| A : array-like or None | ||||||||||||||||
| The values that are mapped to colors. | ||||||||||||||||
|
|
||||||||||||||||
| The base class `.ScalarMappable` does not make any assumptions on | ||||||||||||||||
| the dimensionality and shape of the value array *A*. | ||||||||||||||||
| """ | ||||||||||||||||
| if A is None: | ||||||||||||||||
| self._A = None | ||||||||||||||||
| return | ||||||||||||||||
|
|
||||||||||||||||
| A = _ensure_multivariate_data(A, self.norm.n_components) | ||||||||||||||||
|
|
||||||||||||||||
| A = cbook.safe_masked_invalid(A, copy=True) | ||||||||||||||||
| if not np.can_cast(A.dtype, float, "same_kind"): | ||||||||||||||||
| if A.dtype.fields is None: | ||||||||||||||||
|
|
||||||||||||||||
| raise TypeError(f"Image data of dtype {A.dtype} cannot be " | ||||||||||||||||
| f"converted to float") | ||||||||||||||||
| else: | ||||||||||||||||
| for key in A.dtype.fields: | ||||||||||||||||
| if not np.can_cast(A[key].dtype, float, "same_kind"): | ||||||||||||||||
| raise TypeError(f"Image data of dtype {A.dtype} cannot be " | ||||||||||||||||
| f"converted to a sequence of floats") | ||||||||||||||||
| self._A = A | ||||||||||||||||
| if not self.norm.scaled(): | ||||||||||||||||
| self._colorizer.autoscale_None(A) | ||||||||||||||||
|
|
||||||||||||||||
| def get_array(self): | ||||||||||||||||
| """ | ||||||||||||||||
| Return the array of values, that are mapped to colors. | ||||||||||||||||
|
|
||||||||||||||||
| The base class `.ScalarMappable` does not make any assumptions on | ||||||||||||||||
| the dimensionality and shape of the array. | ||||||||||||||||
| """ | ||||||||||||||||
| return self._A | ||||||||||||||||
|
|
||||||||||||||||
| def changed(self): | ||||||||||||||||
| """ | ||||||||||||||||
| Call this whenever the mappable is changed to notify all the | ||||||||||||||||
| callbackSM listeners to the 'changed' signal. | ||||||||||||||||
| """ | ||||||||||||||||
| self.callbacks.process('changed', self) | ||||||||||||||||
| self.stale = True | ||||||||||||||||
| colorizer = self._get_colorizer(colorizer=colorizer, norm=norm, cmap=cmap) | ||||||||||||||||
| super().__init__(colorizer, **kwargs) | ||||||||||||||||
|
|
||||||||||||||||
| @staticmethod | ||||||||||||||||
| def _check_exclusionary_keywords(colorizer, **kwargs): | ||||||||||||||||
|
|
@@ -710,17 +733,6 @@ def __init__(self, colorizer, **kwargs): | |||||||||||||||
| _api.check_isinstance(Colorizer, colorizer=colorizer) | ||||||||||||||||
| super().__init__(colorizer=colorizer, **kwargs) | ||||||||||||||||
|
|
||||||||||||||||
| @property | ||||||||||||||||
| def colorizer(self): | ||||||||||||||||
| return self._colorizer | ||||||||||||||||
|
|
||||||||||||||||
| @colorizer.setter | ||||||||||||||||
| def colorizer(self, cl): | ||||||||||||||||
| _api.check_isinstance(Colorizer, colorizer=cl) | ||||||||||||||||
| self._colorizer.callbacks.disconnect(self._id_colorizer) | ||||||||||||||||
| self._colorizer = cl | ||||||||||||||||
| self._id_colorizer = cl.callbacks.connect('changed', self.changed) | ||||||||||||||||
|
|
||||||||||||||||
| def _set_colorizer_check_keywords(self, colorizer, **kwargs): | ||||||||||||||||
| """ | ||||||||||||||||
| Raises a ValueError if any kwarg is not None while colorizer is not None. | ||||||||||||||||
|
|
||||||||||||||||
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.
Why Colorbar? Isn't this rather a
ColorMappable?The objects uses a colorizer to map data to colors. It's just a corollary that objects that use colorisers can be associated to a colorbar.
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.
jumping off this comment, I think this object is what would be hooked into for a colormap legend (for example for discrete colormaps) type situation?
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.
Can you please clarify, what is a "colormap legend"?
Without understanding, I'd say no 😛. As said in the comment on the docstring, this object is basically "Data+Colorizer". And colorizer is per-se continous. One can make it look discrete through the (IMHO quite quirky) BoundaryNorm approach. But I wouldn't think about discrete colormaps right now - this abstraction doesn't change however good or bad they are currently supported.
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.
A legend where the handler and label information is fed from a colormap. The most commonly requested use case is probably a scatter plot where folks wanna color the dots based on class. Also useful for heatmaps of classes (somewhat common in ml) and my motivating example was coded data on a heatmap.
Uh oh!
There was an error while loading. Please reload this page.
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.
I gave up on a categorical colormap a very long time ago b/c it kinda broke the cmap/norm abstractions (#6934), so one of my main reasons for being psyched about the colorizer pipeline is it doesn't have those divisions. A CategoricalColorizer that takes in a {category name : color} dict is almost definitely much cleaner to implement than my old hack of listed colormaps and boundary norm approach.
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.
something like this may get simpler in the future. Though it's still somewhat different in that it would target legends not colormaps. So I think this is largely orthogonal to the discussion here.
Coming back to the original topic on naming. I'm now thinking this should even be a
ColorMapper(thinking further @story645 your topic could be aDiscreteColorMapper, which potentially could target legends and colormaps). @trygvrad what do you think on the naming.ColorbarMappable,ColorMappable,ColorMapper?