Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
QuLogic merged 1 commit intomatplotlib:masterfromefiring:pcolor_edges
Feb 17, 2021

Conversation

efiring
Copy link
Member

@efiringefiring commentedSep 14, 2020
edited
Loading

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. Thiscloses#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:

  1. The direct effect of the aliasing is to generate additional setters and getters for all of the aliases. This is done in the Collection base class when the module is imported. All of these additional getters and setters are inherited by subclasses.
  2. The main motivation for the aliasing is to allow singular and plural forms of properties like colors, but abbreviations are also provided. We want to move, though, towards favoring thesingular forms, for consistency with things like Patches and Line2D objects. The directly coded setters and getters are all singular; the aliases are plural or abbreviated. Related private variables are a mixed bag, some singular (e.g.,_original_facecolor), others plural (e.g.,_facecolors).
  3. The signature ofCollection.__init__ includes a trailing**kwargs after a long list of explicit keyword arguments, all of which are presently still theplural 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 callset_facecolorwith the default from the signature (named 'facecolors'), and then it will callset_facecolor` again with the value supplied via the 'facecolor' kwarg.
  4. The purpose of subclassing is to customize a base class, which by itself is typically not usable. The customization can be via adding functionality (new methods), by overriding methods to modify their behavior, or by changing default values. New methods lack aliases unless they are explicitly generated, either directly, or via a decorator on the subclass. Overridden methods are problematic because the aliases are still there, but they point to the implementations in the base class, not to the the overrides in the subclass.
  5. Defaults usually fall into one of two categories: fixed values, like 'black', and values taken from rcParams. The former are unambiguous; the latter can be implemented so that the values are frozen at import time, or so that they are taken at or near the time of instantiation of an object. The latter is preferred. In the context of aliased collection properties, though, this means that they need to be retrieved by a method of the subclass, which is called by the setter defined in Collection.
  6. The way to minimize confusion and clutter with argument handling in the subclass is to let**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 ofLineCollection.__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 forpublic 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

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings andpydocstyle<4 and runflake8 --docstring-convention=all).
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

@efiringefiring marked this pull request as draftSeptember 14, 2020 18:39
@efiringefiring marked this pull request as ready for reviewSeptember 15, 2020 07:11
@QuLogic
Copy link
Member

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 forcolor/colors, for some reason), due todefine_aliases:

@cbook._define_aliases({
"antialiased": ["antialiaseds","aa"],
"edgecolor": ["edgecolors","ec"],
"facecolor": ["facecolors","fc"],
"linestyle": ["linestyles","dashes","ls"],
"linewidth": ["linewidths","lw"],
})
classCollection(artist.Artist,cm.ScalarMappable):

The 'real' oneis singular.

@efiring
Copy link
MemberAuthor

efiring commentedSep 17, 2020 via email

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 somethinglike it, I looked for it. I must have seen it. Oh, well. Theimmediate problem is indeed "color" and "colors", specifically inLineCollection. But I think there is a bit more to it than that.I will take a closer look. The aliasing and kwarg-handling machinery isa bit complicated.

@tacaswelltacaswell added this to thev3.4.0 milestoneSep 23, 2020
@tacaswell
Copy link
Member

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"):
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to extendcbook._str_lower_equal to support multiple values

cbook._str_lower_equal(c, ("none", "face"))?

Copy link
MemberAuthor

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)'.
Copy link
Member

Choose a reason for hiding this comment

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

Do we specify to allowset_array(None)? What should the effect be? AFAICS this is not documented anywhere.

Copy link
MemberAuthor

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)
Copy link
Member

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? Otherwisemapped_colors could be a local variable.

Copy link
MemberAuthor

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.

@efiring
Copy link
MemberAuthor

efiring commentedSep 26, 2020
edited
Loading

(Edited)

We will probably need some discussion of points I added in editing the PR summary:

  1. Maybe in a separate PR, can we do some more removal of plural forms?
  2. Do some other Collection subclasses need simplification of their argument handling, relying on kwargs pass-thru as much as possible, and moving towards keyword-only args?

@efiringefiring mentioned this pull requestSep 26, 2020
7 tasks
@efiring
Copy link
MemberAuthor

I think this is ready to go now. The questions that I raised above can be addressed in subsequent PRs, if desired.

@efiring
Copy link
MemberAuthor

I think I have addressed everything in both reviews. I don't know why codecov/project/tests is unhappy.

@QuLogic
Copy link
Member

I think codecov is complaining about this one condition:
image
However, I think there must be an off-by-one error, as the colouring does not make sense to me.

I think the correct coverage error is that theif not isinstance(fc, str): is never True (thus it would be yellow for partial branch coverage), and thenfc = 'array' is never covered (thus red). That being said, it's failing on the tests part, not the library part, so I could be entirely wrong.

@efiringefiring marked this pull request as draftOctober 20, 2020 19:43
@efiring
Copy link
MemberAuthor

I see another problem, so i'm rethinking this.

@efiringefiring marked this pull request as ready for reviewDecember 24, 2020 18:45
@efiring
Copy link
MemberAuthor

To compare the difference between this PR and v3.3.3 I used the following:

importnumpyasnpimportmatplotlib.pyplotaspltimportmatplotlibver='New'if'post'inmatplotlib.__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'forminTrue,False:fig,axs=plt.subplots(nrows=4,ncols=3,sharex=True,sharey=True,constrained_layout=True)ifm: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'fori, (elab,eopt)inenumerate(edge_options.items()):forj, (flab,fopt)inenumerate(face_options.items()):ax=axs[i,j]pc=ax.pcolormesh(z,edgecolors=eopt,facecolors=fopt,linewidths=5)ifnotm: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:
Old_pcolormesh_with
and this PR yields:
New_pcolormesh_with
The differences are all for the case wherefacecolor='none'. The bottom panel in the middle column shows the new behavior as requested in#1302. The other difference is in the panel above that, where the peculiar combination offacecolor='none', edgecolor='face' now renders neither faces nor edges. This seems reasonable; the user is saying they don't want the face colored, and they want the edge colored the same as the face--i.e., 'none'.

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 forfacecolor), otherwise try to map the edges". Now the rule is "Mapping overrides explicit colors forfacecolor but an explicit color foredgecolor overrides mapping for edges".

lzkelley reacted with heart emoji

@efiring
Copy link
MemberAuthor

@QuLogic I think these failures are in the test system, not in this PR, correct?

@QuLogic
Copy link
Member

Yes, should be,#19301.

@efiring
Copy link
MemberAuthor

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 viaset_array(None), points to another area that might be clarified in a future PR--but not here. The current anomaly is thatset_array, unlike otherScalarMappable setters, does not callchanged(), soset_array(None) does not take effect until something else triggers a draw. Adding thechanged() call toset_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.

Copy link
Member

@jklymakjklymak left a comment
edited
Loading

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...

@jklymakjklymak added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelFeb 1, 2021
@jklymak
Copy link
Member

@brunobeltran you felt you would have bandwidth to review? Thanks!

@QuLogic
Copy link
Member

Well, I'm happy to merge once tests pass, unless you wanted to squash.

@efiring
Copy link
MemberAuthor

Squashed.

@efiring
Copy link
MemberAuthor

The failures are unrelated to this PR--some sort of luatex/pdf/pgf problem.

@tacaswell
Copy link
Member

We restarted the test, will merge as soon as it is green.

@QuLogicQuLogic merged commit14ecfcc intomatplotlib:masterFeb 17, 2021
QuLogic added a commit to QuLogic/matplotlib that referenced this pull requestMar 30, 2021
These were added inmatplotlib#18480, and not copying them can mean that an artistthat used `update_from` could get in an inconsistent state.For example, this will fix legends of colour-mapped scatter plots, wherea copy of the scatter Collection is made to go in the legend.Fixesmatplotlib#19779.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@jklymakjklymakjklymak approved these changes

@brunobeltranbrunobeltranAwaiting requested review from brunobeltran

@timhoffmtimhoffmAwaiting requested review from timhoffm

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.topic: collections and mappables
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

pcolormesh bug: edgecolor ignored
5 participants
@efiring@QuLogic@tacaswell@jklymak@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp