-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Clarify color priorities in collections #18480
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
Conversation
Which part do you mean? All Collections accept both (except for matplotlib/lib/matplotlib/collections.py Lines 22 to 29 in 5584ba7
The 'real' one is singular. |
On 2020/09/15 5:46 PM, Elliott Sales de Andrade wrote:
I found that mplot3d was supplying the singular form of kwargs
(e.g., 'edgecolor' instead of 'edgecolors') to the LineCollection
constructor, which, like all of the Collections, accepts only the
plural form. I added a few lines to the LineCollection constructor
to allow this. It might make sense to do the same for Collection.
Which part do you mean? All Collections accept both (except for
|color|/|colors|, for some reason), due to |define_aliases|:
https://github.com/matplotlib/matplotlib/blob/5584ba7648b1ab258254fd516977cc15b1ae66f7/lib/matplotlib/collections.py#L22-L29
The 'real' one /is/ singular.
Thanks. I don't know how I missed that--I thought there was something
like it, I looked for it. I must have seen it. Oh, well. The
immediate problem is indeed "color" and "colors", specifically in
LineCollection. But I think there is a bit more to it than that.
I will take a closer look. The aliasing and kwarg-handling machinery is
a bit complicated.
|
At least all of the complexity is in one place... |
# We pass through a default value for use in LineCollection. | ||
# This allows us to maintain None as the default indicator in | ||
# _original_edgecolor. | ||
if isinstance(c, str) and c.lower() in ("none", "face"): |
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.
Do we want to extend cbook._str_lower_equal
to support multiple values
cbook._str_lower_equal(c, ("none", "face"))
?
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'm not motivated to do so. I'm not a fan of cbook._str_lower_equal as a replacement for a simple one-liner.
self._facecolors = self.to_rgba(self._A, self._alpha) | ||
elif self._is_stroked: | ||
self._edgecolors = self.to_rgba(self._A, self._alpha) | ||
# Allow possibility to call 'self.set_array(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.
Do we specify to allow set_array(None)
? What should the effect be? AFAICS this is not documented anywhere.
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.
Self._A starts out as None, in which case there is no color mapping; it is optional for a ScalarMappable. Mapping is always initiated via a call to set_array, as far as I know. By allowing the None argument, we can turn mapping on and off again. Increasing this reversibility, the ability to use None to get back to a default state, is part of what I am trying to do in this PR.
'array is dropped.') | ||
# pcolormesh, scatter, maybe others flatten their _A | ||
self._alpha = self._alpha.reshape(self._A.shape) | ||
self._mapped_colors = self.to_rgba(self._A, self._alpha) |
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 it happen that we don't update the array but need to update the facecolors or edgecolors? Otherwise mapped_colors
could be a local variable.
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.
Saving it means we don't have to recalculate it when switching from mapping face to mapping edges, for example.
(Edited) We will probably need some discussion of points I added in editing the PR summary:
|
I think this is ready to go now. The questions that I raised above can be addressed in subsequent PRs, if desired. |
I think I have addressed everything in both reviews. I don't know why codecov/project/tests is unhappy. |
I see another problem, so i'm rethinking this. |
8fe1b2a
to
a3e04d9
Compare
To compare the difference between this PR and v3.3.3 I used the following: import numpy as np
import matplotlib.pyplot as plt
import matplotlib
ver = 'New' if 'post' in matplotlib.__version__ else 'Old'
edge_options = dict(default=None, none='none', face='face', ec='red')
face_options = dict(default=None, none='none', fc='cyan')
z = np.arange(12).reshape((3, 4))
func = 'pcolormesh'
for m in True, False:
fig, axs = plt.subplots(nrows=4, ncols=3, sharex=True, sharey=True,
constrained_layout=True)
if m:
fig.suptitle(f'{ver}: {func} with mappable array')
fname = f'{ver}_{func}_with.png'
else:
fig.suptitle(f'{ver}: {func} without mappable array')
fname = f'{ver}_{func}_without.png'
for i, (elab, eopt) in enumerate(edge_options.items()):
for j, (flab, fopt) in enumerate(face_options.items()):
ax = axs[i, j]
pc = ax.pcolormesh(z, edgecolors=eopt, facecolors=fopt,
linewidths=5)
if not m:
pc.set_array(None)
ax.set_title(f"f:{fopt}, e:{eopt}")
fig.savefig(fname) Looking at figures with a difference, v3.3.3 yields: Apart from those two cases, both of which are somewhat odd corner cases, current behavior is maintained, for better or worse, as far as I can see. The new behavior from #1302 is actually a bit inconsistent with the original general rule, which was "if a mappable array is set, use mapped colors for the face if possible (overriding any explicit color set for |
@QuLogic I think these failures are in the test system, not in this PR, correct? |
Yes, should be, #19301. |
This is rebased with a whats_new entry added. I think it is ready for final (I hope) review. It is intended as an incremental improvement in clarity and consistency. The first item in the whats_new, the ability to turn mapping off via |
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.
This looks good to me - one small suggestion for the tests, but I think the rest is correct...
Marking release critical because it has sat for a while, and looks very close...
@brunobeltran you felt you would have bandwidth to review? Thanks! |
Well, I'm happy to merge once tests pass, unless you wanted to squash. |
d710062
to
4c3d644
Compare
Squashed. |
The failures are unrelated to this PR--some sort of luatex/pdf/pgf problem. |
We restarted the test, will merge as soon as it is green. |
These were added in matplotlib#18480, and not copying them can mean that an artist that used `update_from` could get in an inconsistent state. For example, this will fix legends of colour-mapped scatter plots, where a copy of the scatter Collection is made to go in the legend. Fixes matplotlib#19779.
PR Summary
Color handling in collections is confusing: there are faces and edges, each of which can have a fixed color or a mapped color. Fixed colors (which can be single specifications or arrays matching the elements in the collection) are always available via initialization with defaults. If the collection has a data array set, then it will be used to map colors for edges and/or faces, depending on how their respective fixed color attributes are set. The purpose of this PR is to clarify the logic by consolidating it in a private function. This closes #1302, and is an alternative to #12226.
Edited, 2020-09-25
Working with the collections code is complicated by the aliasing combined with the subclassing. Here's how it looks to me:
_original_facecolor
), others plural (e.g.,_facecolors
).Collection.__init__
includes a trailing**kwargs
after a long list of explicit keyword arguments, all of which are presently still the plural form. When keywords are supplied to the__init__
of a subclass, which always either inherits or callsCollection.__init__
, any of those keywords that match the explicit signature are handled first, along with any un-supplied defaults from the signature. Then, any that remain are handled through theupdate
method inherited fromArtist
, which calls the setter, if any, matching the name of the kwarg. For example, if the supposedly favored 'facecolorkwarg is supplied, the initialization will first call
set_facecolorwith the default from the signature (named 'facecolors'), and then it will call
set_facecolor` again with the value supplied via the 'facecolor' kwarg.**kwargs
handle everything that does not require a subclass-specific fixed default.I think this PR is now consistent with the above points with respect to LineCollection.
A possible objection is that by trimming down the signature of
LineCollection.__init__
, we are breaking any code using positional calling for properties that were in the signature. I think we should move in this direction anyway, making such properties keyword-only. I have added a compatibility shim with a deprecation warning to handle existing code.It also appears to me that confusion could be reduced with no backwards incompatibility for public attributes if we were to change all the signature forms to singular ('facecolors' -> 'facecolor'), and similarly for the private attributes that are still plural.
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
andpydocstyle<4
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).