MNT: set the facecolor of nofill markers

[tacaswell]

PR Summary

Even though we are going to ignore it, set the facecolors to the user
specified color and edgecolor to 'face' to maintain
back-compatibility.

We now warn if the user passes in an edgecolor that we ignore.

closes #17849

partially reverts #17543 / d86cc2b

Do we want an API change note for the warning?

PR Checklist

• Has Pytest style unit tests
• Code is Flake 8 compliant
• Documentation is sphinx and numpydoc compliant
• Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

[timhoffm]

I don‘t think this is the right solution. Explanation follows later today.

[timhoffm]

Sorry for being a bit terse above. I didn't have time to think it though completely and write up details, but already wanted to announce that the rabbit hole is deeper.

We have three cases of markers:

1. Regular filled markers with a fill color (e.g. maker='o')
2. Only the outlines of 1. if their fillstyle is set to 'none' (MarkerStyle(marker='o', fillstyle='none'))
3. Markers that cannot be filled (e.g. marker='x')

---

Original bug and fixed version

The original bug #17527 is: scatter() draws case 2 with filling.
This was fixed via #17543:

before:

Note that the MarkerStyle row should have unfilled circles.

after:

Click for code of the example plots

import numpy as np
import matplotlib.pyplot as plt
from matplotlib import markers
from matplotlib import ticker

x = np.random.random(41)
y = np.random.random(41)

fig, axs = plt.subplots(3, 4)

fc = 'none'
ec = 'tab:orange'

markers = ['o', markers.MarkerStyle(marker='o', fillstyle='none'), 'x']

for i, marker in enumerate(markers):
    axs[i, 0].scatter(x, y, marker=marker)
    axs[i, 1].scatter(x, y, marker=marker, facecolor=fc)
    axs[i, 2].scatter(x, y, marker=marker, edgecolor=ec)
    axs[i, 3].scatter(x, y, marker=marker, facecolor=fc, edgecolor=ec)

axs[0, 0].set_title('no args')
axs[0, 1].set_title('fc')
axs[0, 2].set_title('ec')
axs[0, 3].set_title('fc + ec')
axs[0, 0].set_ylabel("'o'")
axs[1, 0].set_ylabel("MarkerStyle")
axs[2, 0].set_ylabel("'x'")
for ax in axs.flat:
    ax.set_xticks([])
    ax.set_yticks([])

The MarkerStyle row now has unfilled circles. Cases 2 and 3 are handled equivalently, which IMHO makes sense.
Note however, that there is no orange in the bottom two rows because edgecolor is ignored for unfilled markers.

---

Details of the fix

#17543 addresses this in two steps/commits:

• Markers with fillstyle 'none' should return False for is_filled()
  Thinking of it again I'm not sure anymore if this fix is right. One could also interpret is_filled() as "can have a fill color". The returned value for case 2 would be different depending on the interpretation.
• scatter() uses is_filled() as a way of determining whether edgecolor should be ignored.

---

Why only setting the facecolor for nofill markers is not correct.

For the new interpretation MarkerStyle(marker='o', fillstyle='none').is_filled() == False we would again fill the marker.

It may work if we also revert is_filled() to the old behavior ("can have a fill color"), i.e. revert #17543 completely. But I'm not clear how to fix the original bug #17527. Would need additional consideration.

---

General comments and way forward

• IMHO it is ok for now to revert Fix linewidths and colors for scatter() with unfilled markers #17543 and leave the bug The markers are not hollow when I use ax.scatter() and set markers.MarkerStyle()'s fillstyle to 'none'. My usage is wrong? #17527 open for the 3.3 release.
• We should clarify how we interpret is_filled() and document that.
• We should clarify which outputs we expect in the above matrix of markers and coloring parameters.
  In particular, I would interpret both 'x' and unfilled 'o' as only having edges, but no faces. Therefore facecolor should be ignored not edgecolor. It's another aspect if we actually want to go through the deprecation hassle. But it helps to have a clear understanding how it the semantics should ideally be and then see how close we can get. We may need to adapt the color inference logic in _parse_scatter_color_args().
• I just realize that in the above example it makes a difference if I use fc or facecolor (and ec or edgecolor) 😨. But that's a separate story that should be targeted after we have fixed this mess here.

[tacaswell]

I spoke about this with @QuLogic on the phone, we decided that this is something that we need to think about carefully and make sure we cover all the cases so are going to revert #17543 only for v3.3.x and then fix it on master for 3.4.

[QuLogic]

The direct revert to v3.3.x is in #17861.

[QuLogic]

Note, we had a similar to #17543, but not yet merged PR in #10008. There is some discussion there as well.

[tacaswell]

On further consideration I have concluded @timhoffm 's judgement of what the behavior should be was correct. This PR tweaks how we go about implementing it.

From reading the code it seems that marker.is_filled() and maker.get_fillstelye() == 'none' are not always the same (critically for the non-fillable markers!). This adds a special case for fillable makers that the user has asked us not to fill from unfillable markers where we do not worry if the user asked us to fill them because we can not.

This also helps to clarify when we should collapse the edge / face colors. For un-fillable makers we don't have a face so we must collapse them and for back-compatibility reasons we leave the face color alone and set the edge color to 'face'. On the other hand, if the user gave us a fillable makers that they want to not be filled, then we have to set the face color to be 'none' (as that is the only way to prevent PathCollection from filling in the paths).

With the code in this PR we have:

---

If we drop promoting the facecolor to edge color for un-filled-fillable markers then we get

which I think is better, in that it makes the behavior between 'o' and MakerStyle('o', fill_style='none') more consistent. However, the no-args case is pretty bad and I do see the argument that 'x' and MarkerStyle('o', fillstyle='none') should be consistent.

---

It looks like we don't call cbook._normalize_kwargs before we pass off to _parse_scatter_color_args so the short aliases are crashing through to Artist.set. That should in principle be a simple fix, but given how much other special casing we are doing I was hesitant to tack that onto this PR.

[tacaswell]

We discussed this on the call. We either need to adopt this and bank the step forward that was made in #17543 or take a step back and accept the broken state and punt a more through fix (as outlined by @brunobeltran in #10008 's comments) for 3.5.

[timhoffm]

TL;DR

I think this is the right way to go. (May be incremental, but is better than before because it fixes #17527 and does so without breaking seaborn, which my first attempt #17543 did).

Explanation

This PR only concerns the special case of MarkerStyle(..., fillstyle='none').

The interplay of that with colors has been obviously wrong because such a marker should never be filled (see bug #17527).

The implementation in the PR follows the claims in our documentation:

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

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

Whether or not this color mangling is a good choice is a different question, but it has always been working for true unfilled markers plt.scatter(1, 2, marker='x', facecolor='r').
Changing color mangling generally (and further marker characteristics as proposed in #10008 (comment)) is an API change not only a bug fix. That would require much more careful thinking and is definitively not for 3.4.

This PR "only" fixes a bug for MarkerStyle(..., fillstyle='none') and does so in a way consistent with unfilled markers.

Further remarks

As @tacaswell correctly observes marker.is_filled() and maker.get_fillstyle() == 'none' are not necessarily equivalent. Both methods should get a note on this in the docstring and explain more precisely what they mean for for the different kinds of markers.

[timhoffm]

I'm still somewhat confused why we need special-casing in the drawing function. Could this not be handled in the MarkerStyle so that MarkerStyle('o', fillstyle='none') and MarkerStyle('x') have an identical behavior?

[jklymak]

TL;DR

I think this is the right way to go. (May be incremental, but is better than before because it fixes #17527 and does so without breaking seaborn, which my first attempt #17543 did).

Explanation

This PR only concerns the special case of MarkerStyle(..., fillstyle='none').

I'm still concerned about this. It was broken, so we are fixing it here. However, I'm not convinced we want it. While its broken we could instead remove it.

In particular what is the goal of fillstyle='none'? It was added when fillstyle='left' and friends were added. However, currently, if you don't want to have a facecolor, you can also just set facecolor='none'. Having fillstyle also set that seems unnecessary, and an alternative way to go is to just forbid "none" as a possible argument to fillstyle. That strikes me a simpler.

[timhoffm]

In particular what is the goal of fillstyle='none'?

To a certain extend, it is appealing to be able to define "outline-only" markers and then be able to use them like the unfilled markers*. The advantage is that I can define the marker once ring_marker = MarkerStyle('o', fillstyle='none') and then reuse it whenever I want without having to care about setting facecolor additionally. For example consider a GUI that let's you choose your marker. It would be really cumbersome to support a ring marker in there if, the corresponding plot calls would need to be adapted for facecolor.

*This is maybe the point fillstyle='none' markers and unfilled markers should IMHO be equivalent because they are just "strokes". Ping @story645 in case you want to comment on that from a topological point of view. But apparently they are different, so that we have to jump some extra hoops in this PR to make them render their strokes using the same color logic.
See also #17850 (comment).

So my refined opinion:

• We still want fillstyle='none' markers
• The "correct" implementation would be to make them an unfilled marker upon creation.
• If we don't have the time to look into the "correct" solution before 3.4, this PR is still feasible because it gives the desired result, but with a hacky implementation. But we should cleanup afterwards then.
• By the way: I think we should make MarkerStyle objects immutable and remove fillstyle property from Line2D. If one wants to change the fillstyle, one should create a new marker.

But can't we keep it simple and remove fillstyle='none'?

1. We'd loose some functionality, i.e. you don't have ring markers then. You can only mimic them using facecolor='none' but at the expense of coupling the desired shape to coloring.
2. Additionally, fillstyle='none' only has problems with scatter(), AFAIK it works for plot(). Removing it would force users to rewrite working code.

Since we have unfilled markers anyway, it should be relatively straight forward to make fillstyle='none' markers be unfilled markers. That seems less of a dance than telling users to use facecolor='none'.

[mwaskom]

IMHO it would be extremely nice if there were a native "ring marker" in matplotlib

[timhoffm]

IMHO it would be extremely nice if there were a native "ring marker" in matplotlib

Agreed. That's probably the most common use case for MarkerStyle(..., fillstyle='none').

In principle that's not difficult. We could easily add a new marker, say 'r'. I'm just wondering if hard-coding this special case is reasonable. If people then start wanting native open squares then, we're running out of letters at some point. Alternatively, we could introduce a pattern [marker]_ so that the ring marker could be written as 'o_'. The underscore postfix hinting at the outline-only characteristics. OTOH that might be too clever?

[mwaskom]

From a higher-level library perspective, having the ability to set fillstyle="none" on a generic marker is quite useful, and using only the facecolor attribute to make a hollow marker is constraining. But from a user persepctive, empty rings are such a useful marker that having a special-cased native type for it would also be nice.

[story645]

But from a user persepctive, empty rings are such a useful marker that having a special-cased native type for it would also be nice.

I think even from a library perspective, like @timhoffm said unfilled + unfillable markers inherently encode at least 1 fewer variable than filled markers and so I think it could be good to have that cleanly pinned down as a distinct class/set of marker choices.
Basically we've got

• can't be filled, unfilled, latex really only support 1 color, facecolor always equals edgecolor,
• filled supports facecolor, edgecolor, facealtcolor if half filled (which don't like that altcolor is dependent on fill style but 🤷)

so that the ring marker could be written as 'o_'

There's cmap_r and that feels straightforward, I think so long as the convention is documented is fine.

[mwaskom]

Note that there's already a sort of convention for unfilled/filled markers with "x" -> "X" and "+" -> "P", although it's not possible to make it consistent with creating unfilled markers that already have a lowercase marker code.

[timhoffm]

That's not the same. 'P_' would give an outlined thick plus, while '+' is just a horizontal and a vertical line.

[mwaskom]

Hm, good point, from an implementation perspective. Though if I were representing a variable by mapping it to marker type, I would likely pair a filled circle and plus with a hollow circle and line-art plus.

[timhoffm]

Sorry hit the wrong button ... 🙄

[timhoffm]

sort of convention for unfilled/filled markers with "x" -> "X" and "+" -> "P"

This is more or less coincidence and does not have anything to do with removing the filling.

Though if I were representing a variable by mapping it to marker type, I would likely pair a filled circle and plus with a hollow circle and line-art plus.

I'm not trying to pair anything here. We're having MarkerStyle([marker], fillstyle='none') and the proposal is to make these easier accessible via the string "[marker]_".

[mwaskom]

Right, I understand the implementation details. I am saying that there exists a quasi-convention for doing something similar (and something that might appear identical from the perspective of someone who has not thought through the implementation details), and it may be useful to keep that in mind when you add a new convention.

[timhoffm]

I understand your point. However, IMHO this "quasi-convention" cannot be applied to more cases. It only works if the filled marker looks like "made of strokes". Among the filled markers we have this is only the case for the exact two cases you mentioned. All others are more "solid" shapes and I couldn't come up with a reasonable corresponding unfilled maker, except one tracing their outline. So I think there is no way to take the quasi-convention into account for more cases. '+' and 'x' are just two exceptions.

[tacaswell]

@timhoffm can you clear your review?

[jklymak]

This seems to be genuinely failing:

Unexpected failing examples:
/home/circleci/project/examples/animation/rain.py failed leaving traceback:
Traceback (most recent call last):
  File "/home/circleci/project/examples/animation/rain.py", line 40, in <module>
    scat = ax.scatter(rain_drops['position'][:, 0], rain_drops['position'][:, 1],
  File "/home/circleci/project/lib/matplotlib/__init__.py", line 1405, in inner
    return func(ax, *map(sanitize_sequence, args), **kwargs)
  File "/home/circleci/project/lib/matplotlib/axes/_axes.py", line 4494, in scatter
    orig_edgecolor = edgecolors or kwargs.get('edgecolor', None)
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

not passing

[jklymak]

The example passes if you do this. I guess that means we need an extra text where edgecolors is an array.