Problems caused by changes to logic of scatter coloring in matplotlib 3.3.0.rc1 · Issue #17849 · matplotlib/matplotlib · GitHub

Bug report

Bug summary

Not a bug as much as a change in behavior that causes downstream issues.

#17543 changes the values set for the facecolors and edgecolors of "unfilled" marker in scatter.

These changes break seaborn, which relies in several places on scatter setting facecolors for both filled and unfilled markers.

seaborn can work around the issue in a new release, but at the moment there is no user-facing workaround I can suggest, and the changes mean that all seaborn scatterplots with an unfilled marker will be blank.

Code for reproduction

import matplotlib.pyplot as plt
plt.scatter([0], [0], marker="+").get_facecolors()

Actual outcome

With 3.3.0.rc1

array([], shape=(0, 4), dtype=float64)

Expected outcome

With 3.2.2

array([[0.12156863, 0.46666667, 0.70588235, 1.        ]])

xref mwaskom/seaborn#2159

Ping @tacaswell @QuLogic.

Should we back out #17543 (at least the second commit) from 3.3 and target for 3.4 to give seaborn time to adapt? It is the right way of handling things but it is not super critical; IMHO it's better to keep the old behavior for a version and not introduce non-user-fixable issues with current seaborn and mpl 3.3.

seaborn-specific issues aside, I actually find the new behavior really confusing. It's surprising that specifying facecolor in the function call sets the edgecolor (and nulls out the facecolor) attribute of the resulting collection.

c = plt.scatter([0], [0], marker="+", s=500, facecolor="r")
print(c.get_facecolors())
print(c.get_edgecolors())

[]
[[1. 0. 0. 1.]]

Dispatching color= based on the marker type makes sense , but this seems to violate what the caller asked for.

I agree that this behavior is questionable, but AFAICS this has been the case before and has not been changed by #17543.

https://matplotlib.org/3.2.2/api/_as_gen/matplotlib.axes.Axes.scatter.html :

On 3.2.2 I get (slightly modified example)

import matplotlib.pyplot as plt
c = plt.scatter([0], [0], marker="+", s=500, facecolor="r", edgecolor="b")
print(c.get_facecolors())
print(c.get_edgecolors())

[[1. 0. 0. 1.]]
[[1. 0. 0. 1.]]

whereas on 3.3.0.rc1 the output is

[]
[[1. 0. 0. 1.]]

So I guess you're right that the behavior was always a little surprising. But the old behavior is at least quasi-consistent (you can use facecolor to set and/or read the color of the lines), whereas the new behavior is very difficult to explain: "facecolor determines the color of the marker, but it sets the edgecolor attribute of the collection".

The fundamental problem with your observed inconsistency is still

for non-filled markers, the edgecolors kwarg is ignored and forced to 'face' internally.

This was probably done in the notion of primary color (marker='o' is red if it is filled red, marker='+' is red if the lines are red), but is poorly mapped on the concept of facecolor/edgecolor.

The "correct" solution from the library point of view would be to ignore facecolor and respect edgecolor for unfilled markers. But that'll likely break much more code than the above change in collection.get_facecolor(). I don't dare this API change.

From a user point of view plt.scatter([0], [0], marker="+", s=500, facecolor="r", edgecolor="b") is already ill defined and digs into consistency weaknesses of the API. I suggest to use plt.scatter([0], [0], marker="+", s=500, color="r"); color being in the notion of primary color.

We should definitely un break seaborn.

I get the root problem, I’m just saying that a good principle might be that function kwargs should round-trip through the artist attributes that they govern.

MNT: set the facecolor of nofill markers

We can't literally round-trip input values because we do a fair amount of internal validation / normalization / broadcasting and use the public API between the different parts of Matplotlib. For example, with scatter you can pass in the edge color 'face' which is not something you want to get back out of sc.get_edgecolor().

so if I am reading this right the old behavior for unfilled markers was

the user supplied edgecolor (if given) was ignored and set to 'face'
the color / facecolor was set to face color
together this mean that get_facecolors and get_edgecolors came out the same

The new behavior is:

the user supplied edgecolor is still ignored
"none" is set to the facecolor (which mean sc.get_facecolors() comes back empty)
the facecolor passed in by the user is set to the edgecolor

I think the core of the confusion is that if, knowing nothing of our internals you were shown at "un filled marker" and asked if "facecolor" well described the color of the marker you would probably say "yes" (because it make more sense to have a face but no edge, then and edge but no face) so at some point in the past we up-converted facecolor -> edgecolor in this case.

I think the behavior should be:

for un-filled markers we set both edgecolor and facecolor to be the same thing (to maintain back compat and because it seems like a reasonable way to square this particular circle even if it does not round-trip correctly (setting the face color will do nothing, maybe that is something we should fix?))
warn if we are going to ignore the user supplied edgecolor, but not decide if we are going to deprecate that in the future (yet) so we have a bit of time to think about how we should settle between facecolor / edgecolor when we can only use one of them.

We can't literally round-trip input values because we do a fair amount of internal validation / normalization / broadcasting and use the public API between the different parts of Matplotlib.

https://twitter.com/michaelwaskom/status/1280544008104706049

Right, of course.

I think the behavior should be:

for un-filled markers we set both edgecolor and facecolor to be the same thing (to maintain back compat and because it seems like a reasonable way to square this particular circle even if it does not round-trip correctly (setting the face color will do nothing, maybe that is something we should fix?))

See #17850 (comment) for why we need to go deeper.

The key part was reverted for 3.3.0, so this issue itself is no longer release critical for it.

MNT: set the facecolor of nofill markers

MNT: use get_fillstyle not is_filled to null facecolor

Problems caused by changes to logic of scatter coloring in matplotlib 3.3.0.rc1 · Issue #17849 · matplotlib/matplotlib · GitHub

Repository navigation

Problems caused by changes to logic of scatter coloring in matplotlib 3.3.0.rc1 #17849

Bug report

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Problems caused by changes to logic of scatter coloring in matplotlib 3.3.0.rc1 · Issue #17849 · matplotlib/matplotlib · GitHub

Repository navigation

Problems caused by changes to logic of scatter coloring in matplotlib 3.3.0.rc1 #17849

Description

Bug report

Activity

timhoffm commented on Jul 6, 2020

mwaskom commented on Jul 6, 2020

timhoffm commented on Jul 6, 2020

mwaskom commented on Jul 6, 2020

timhoffm commented on Jul 6, 2020

tacaswell commented on Jul 7, 2020

mwaskom commented on Jul 7, 2020

tacaswell commented on Jul 7, 2020

mwaskom commented on Jul 7, 2020

timhoffm commented on Jul 7, 2020

QuLogic commented on Jul 9, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions