Skip to content

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

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

efiring
Copy link
Member

@efiring efiring commented Sep 14, 2020

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:

  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 the singular 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 of 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 calls Collection.__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 the update method inherited from Artist, 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 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

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@efiring efiring marked this pull request as draft September 14, 2020 18:39
@efiring efiring marked this pull request as ready for review September 15, 2020 07:11
@QuLogic
Copy link
Member

QuLogic commented Sep 16, 2020

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:

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

The 'real' one is singular.

@efiring
Copy link
Member Author

efiring commented Sep 17, 2020 via email

@tacaswell tacaswell added this to the v3.4.0 milestone Sep 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 extend cbook._str_lower_equal to support multiple values

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

Copy link
Member Author

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 allow set_array(None)? What should the effect be? AFAICS this is not documented anywhere.

Copy link
Member Author

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

Copy link
Member Author

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
Member Author

efiring commented Sep 26, 2020

(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?

@efiring efiring mentioned this pull request Sep 26, 2020
7 tasks
@efiring
Copy link
Member Author

efiring commented Sep 28, 2020

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
Member Author

efiring commented Oct 18, 2020

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

@QuLogic
Copy link
Member

QuLogic commented Oct 20, 2020

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 the if not isinstance(fc, str): is never True (thus it would be yellow for partial branch coverage), and then fc = '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.

@efiring efiring marked this pull request as draft October 20, 2020 19:43
@efiring
Copy link
Member Author

efiring commented Oct 20, 2020

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

@efiring efiring marked this pull request as ready for review December 24, 2020 18:45
@efiring efiring requested a review from brunobeltran December 24, 2020 18:46
@efiring
Copy link
Member Author

efiring commented Dec 24, 2020

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:
Old_pcolormesh_with
and this PR yields:
New_pcolormesh_with
The differences are all for the case where facecolor='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 of facecolor='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 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".

@efiring
Copy link
Member Author

efiring commented Jan 15, 2021

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

@QuLogic
Copy link
Member

QuLogic commented Jan 15, 2021

Yes, should be, #19301.

@efiring
Copy link
Member Author

efiring commented Jan 18, 2021

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.

Copy link
Member

@jklymak jklymak left a 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...

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 1, 2021
@jklymak
Copy link
Member

jklymak commented Feb 4, 2021

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

@QuLogic
Copy link
Member

QuLogic commented Feb 17, 2021

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

@efiring
Copy link
Member Author

efiring commented Feb 17, 2021

Squashed.

@efiring
Copy link
Member Author

efiring commented Feb 17, 2021

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.

@QuLogic QuLogic merged commit 14ecfcc into matplotlib:master Feb 17, 2021
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Mar 30, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: collections and mappables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pcolormesh bug: edgecolor ignored
5 participants