-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MNT: set the facecolor of nofill markers #17850
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don‘t think this is the right solution. Explanation follows later today.
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:
Original bug and fixed versionThe original bug #17527 is: before: 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. Details of the fix#17543 addresses this in two steps/commits:
Why only setting the facecolor for nofill markers is not correct.For the new interpretation It may work if we also revert General comments and way forward
|
The direct revert to |
55e16ca
to
0f6bcd1
Compare
0f6bcd1
to
574a1fc
Compare
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 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 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 It looks like we don't call |
574a1fc
to
c582e83
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# | ||
# While not an ideal situation, but is better than the | ||
# alternatives. | ||
if marker_obj.get_fillstyle() == 'none': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 |
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 *This is maybe the point So my refined opinion:
But can't we keep it simple and remove
|
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 In principle that's not difficult. We could easily add a new marker, say |
From a higher-level library perspective, having the ability to set |
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.
There's cmap_r and that feels straightforward, I think so long as the convention is documented is fine. |
Note that there's already a sort of convention for unfilled/filled markers with |
That's not the same. 'P_' would give an outlined thick plus, while '+' is just a horizontal and a vertical line. |
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. |
Sorry hit the wrong button ... 🙄 |
This is more or less coincidence and does not have anything to do with removing the filling.
I'm not trying to pair anything here. We're having |
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. |
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. |
47b9c19
to
a89443c
Compare
@timhoffm can you clear your review? |
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example passes if you do this. I guess that means we need an extra text where edgecolors is an array.
This is brittle, but matches the behavior in Line2D. MarkerStyle objects have two coupled, but not fully redundant methods for determining if the maker is filled: the `is_filled` and `get_fillstyle` methods. If `ms.get_fillstyle() == 'none'` then `ms.is_filled() is False`, however the converse is not True. In particular the markers that can not be filled (because the Paths they are made out of can not be closed) have `ms.get_fillstyle() == 'full'` and `ms.is_filled() is False`. In Line2D we filter on the value of `get_fillstyle` not on `is_filled` so do the same in `Axes.scatter`. In Line2D we do the validation at draw time (because Line2D holds onto its MarkerStyle object instead of just extracting the path). The logic for fillstyle on Markers came in via matplotlib#447/ 213459e. closes matplotlib#17849 Revises matplotlib#17543 / d86cc2b Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> Co-authored-by: Jody Klymak <jklymak@gmail.com> Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
2e11021
to
d206a0e
Compare
To maintain back-compatibility, matplotlib decided to ignore the issue. fixes #100 Ref: matplotlib/matplotlib#10008, matplotlib/matplotlib#17850
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