Conversation
e418ac7 to
b530be5
Compare
|
|
||
|
|
||
| class _ColorizerInterface: | ||
| class _ColorbarMappable: |
There was a problem hiding this comment.
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.
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.
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.
what is a "colormap legend"?
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.
There was a problem hiding this comment.
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.
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 a DiscreteColorMapper, which potentially could target legends and colormaps). @trygvrad what do you think on the naming. ColorbarMappable, ColorMappable, ColorMapper?
| Base class that contains the interface to `Colorizer` objects from | ||
| a `ColorizingArtist` or `.cm.ScalarMappable`. |
There was a problem hiding this comment.
| Base class that contains the interface to `Colorizer` objects from | |
| a `ColorizingArtist` or `.cm.ScalarMappable`. | |
| Base class for objects that maps data to colors via a `Colorizer`. | |
| This is e.g. the base for `ColorizingArtist` or `.cm.ScalarMappable`. |
I think the subsequent note needs allow an update. My understanding is that this class is basically "Data+Colorizer" and related functionality like changed.
| @property | ||
| def axes(self): | ||
| return self._axes | ||
|
|
||
| @axes.setter | ||
| def axes(self, axes): | ||
| self._axes = axes |
There was a problem hiding this comment.
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.
axes is not used anywhere in the class itself. If we want to nevertheless add this here, we need a good reason for that.
There was a problem hiding this comment.
This is a good point. Instead of having this here we can have a hasattr(mappable, 'axes') in colorbar().
There was a problem hiding this comment.
We could make a very nuanced sematnic distinction here: If you keep the original name _ColorbarMappable, we could make the operational definition as "some object that can be passed to a colorbar". That would allow to add whatever is needed, including an optional axes.
OTOH I think I still like the conceptual definition "_ColorMappable = Data + Colorizer" more, even if this means colorbar() has to jump an extra hoop with hasattr(mappable, 'axes') (or possibly isinstance(mappable, Artist) - without having checked I assume that colorbar will do some thing additional for Artists in an Axes, that is not needed/possible for an abstract mapping). I propose to go this direction.
From a quick check, the only usage of axes in Colorbar seems to be
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.
There was a problem hiding this comment.
Including can_navigate() makes perfect sense :)
@timhoffm @story645 there is a thought I want to run by you.
I believe all classes that subclass ColorizingArtist will set .axes, because this is a requirement for fig.colorbar(mappable) to behave as expected. However, there is no mention of an .axes attribute in ColorizingArtist or the classes it inherits from.
To me, it seems like .axes is an important attribute of ColorizingArtist, and it should therefore be converted to a property, such that its existence is included in the documentation for ColorizingArtist and mentioned in colorizer.py. What do you think about this?
There was a problem hiding this comment.
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 axes, but that can and should be done as a separate action.
There was a problem hiding this comment.
I think every Artist has an axes attribute.
This is likely unrelated, but do figure level artists just have axes set to none?
There was a problem hiding this comment.
Yes, but it's more complicated behind the scenes. Let's move the topic to #31118.
The ticks and labels along the colorbas are still not centered vertically in the middle but otherwise coloring works. Maybe once matplotlib/matplotlib#31039 https://github.com/orgs/matplotlib/projects/9/views/1 are implemented as a new DiscreteColorMapper. https://gist.github.com/jakevdp/91077b0cae40f8f8244a matplotlib/matplotlib#31178 matplotlib/matplotlib#28317 matplotlib/matplotlib#28763 matplotlib/matplotlib#28349 Some helpful links: https://stackoverflow.com/questions/13784201/how-to-have-one-colorbar-for-all-subplots https://stackoverflow.com/questions/14770735/how-do-i-change-the-figure-size-with-subplots https://www.geeksforgeeks.org/python/how-to-create-different-subplot-sizes-in-matplotlib/ https://thelinuxcode.com/how-to-change-the-number-of-ticks-in-matplotlib-a-practical-modern-guide/
The ticks and labels along the colorbas are still not centered vertically in the middle but otherwise coloring works. Maybe once matplotlib/matplotlib#31039 https://github.com/orgs/matplotlib/projects/9/views/1 are implemented as a new DiscreteColorMapper. https://gist.github.com/jakevdp/91077b0cae40f8f8244a matplotlib/matplotlib#31178 matplotlib/matplotlib#28317 matplotlib/matplotlib#28763 matplotlib/matplotlib#28349 Some helpful links: https://stackoverflow.com/questions/13784201/how-to-have-one-colorbar-for-all-subplots https://stackoverflow.com/questions/14770735/how-do-i-change-the-figure-size-with-subplots https://www.geeksforgeeks.org/python/how-to-create-different-subplot-sizes-in-matplotlib/ https://thelinuxcode.com/how-to-change-the-number-of-ticks-in-matplotlib-a-practical-modern-guide/
The ticks and labels along the colorbas are still not centered vertically in the middle but otherwise coloring works. Maybe once matplotlib/matplotlib#31039 https://github.com/orgs/matplotlib/projects/9/views/1 are implemented as a new DiscreteColorMapper. https://gist.github.com/jakevdp/91077b0cae40f8f8244a matplotlib/matplotlib#31178 matplotlib/matplotlib#28317 matplotlib/matplotlib#28763 matplotlib/matplotlib#28349 Some helpful links: https://stackoverflow.com/questions/13784201/how-to-have-one-colorbar-for-all-subplots https://stackoverflow.com/questions/14770735/how-do-i-change-the-figure-size-with-subplots https://www.geeksforgeeks.org/python/how-to-create-different-subplot-sizes-in-matplotlib/ https://thelinuxcode.com/how-to-change-the-number-of-ticks-in-matplotlib-a-practical-modern-guide/

This PR renames
_ColorizerInterfaceto_ColorbarMappableand shifts some functionality such that_ColorbarMappablecan function as the mappable forfig.colorbar(mappable)._ColorbarMappablediffers from aColorizingArtist[the regular input tofig.colobar()] in that it is not anArtist.This change is needed for improvements to
Axes3d.voxel(), see #31032, but may also find other uses.I don't think there is currently any use case for
_ColorizerInterfaceoutside of ColorizingArtistThis PR only changes the private API, with no changes to the public API.
PR checklist