| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Which part do you mean? All Collections accept both (except for color/colors, for some reason), due to define_aliases: matplotlib/lib/matplotlib/collections.py Lines 22 to 29 in 5584ba7 The 'real' one is singular. |
Sorry, something went wrong.
|
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.
|
Sorry, something went wrong.
At least all of the complexity is in one place... |
Sorry, something went wrong.
| # 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.
Do we want to extend cbook._str_lower_equal to support multiple values
cbook._str_lower_equal(c, ("none", "face"))?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| 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.
Do we specify to allow set_array(None)? What should the effect be? AFAICS this is not documented anywhere.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| '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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Saving it means we don't have to recalculate it when switching from mapping face to mapping edges, for example.
Sorry, something went wrong.
|
(Edited) We will probably need some discussion of points I added in editing the PR summary:
|
Sorry, something went wrong.
|
I think this is ready to go now. The questions that I raised above can be addressed in subsequent PRs, if desired. |
Sorry, something went wrong.
|
I think I have addressed everything in both reviews. I don't know why codecov/project/tests is unhappy. |
Sorry, something went wrong.
Sorry, something went wrong.
|
I see another problem, so i'm rethinking this. |
Sorry, something went wrong.
|
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 facecolor), otherwise try to map the edges". Now the rule is "Mapping overrides explicit colors for facecolor but an explicit color for edgecolor overrides mapping for edges". |
Sorry, something went wrong.
|
@QuLogic I think these failures are in the test system, not in this PR, correct? |
Sorry, something went wrong.
|
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 set_array(None), points to another area that might be clarified in a future PR--but not here. The current anomaly is that set_array, unlike other ScalarMappable setters, does not call changed(), so set_array(None) does not take effect until something else triggers a draw. Adding the changed() call to set_array breaks contouring, however, so an attempt to address this anomaly leads down a rabbit hole that I don't want the current PR to enter. |
Sorry, something went wrong.
There was a problem hiding this comment.
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...
Sorry, something went wrong.
|
@brunobeltran you felt you would have bandwidth to review? Thanks! |
Sorry, something went wrong.
|
Well, I'm happy to merge once tests pass, unless you wanted to squash. |
Sorry, something went wrong.
|
Squashed. |
Sorry, something went wrong.
|
The failures are unrelated to this PR--some sort of luatex/pdf/pgf problem. |
Sorry, something went wrong.
|
We restarted the test, will merge as soon as it is green. |
Sorry, something went wrong.
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.
| Back | FazBrowse Home | New Git URL |
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:
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