MNT: use get_fillstyle not is_filled to null facecolor

tacaswell · timhoffm · jklymak · tacaswell · commit d206a0e92a6d · 2021-01-28T19:19:28.000-05:00
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 #447/ 213459e. closes #17849 Revises #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>
diff --git a/lib/matplotlib/axes/_axes.py b/lib/matplotlib/axes/_axes.py
@@ -4490,6 +4490,10 @@ def scatter(self, x, y, s=None, c=None, marker=None, cmap=None, norm=None,
                 "s must be a scalar, "
                 "or float array-like with the same size as x and y")
 
+        # get the original edgecolor the user passed before we normalize
+        orig_edgecolor = edgecolors
+        if edgecolors is None:
+            orig_edgecolor = kwargs.get('edgecolor', None)
         c, colors, edgecolors = \
             self._parse_scatter_color_args(
                 c, edgecolors, kwargs, x.size,
@@ -4518,6 +4522,36 @@ def scatter(self, x, y, s=None, c=None, marker=None, cmap=None, norm=None,
         path = marker_obj.get_path().transformed(
             marker_obj.get_transform())
         if not marker_obj.is_filled():
+            if orig_edgecolor is not None:
+                _api.warn_external(
+                    f"You passed a edgecolor/edgecolors ({orig_edgecolor!r}) "
+                    f"for an unfilled marker ({marker!r}).  Matplotlib is "
+                    "ignoring the edgecolor in favor of the facecolor.  This "
+                    "behavior may change in the future."
+                )
+            # We need to handle markers that can not be filled (like
+            # '+' and 'x') differently than markers that can be
+            # filled, but have their fillstyle set to 'none'.  This is
+            # to get:
+            #
+            #  - respecting the fillestyle if set
+            #  - maintaining back-compatibility for querying the facecolor of
+            #    the un-fillable markers.
+            #
+            # While not an ideal situation, but is better than the
+            # alternatives.
+            if marker_obj.get_fillstyle() == 'none':
+                # promote the facecolor to be the edgecolor
+                edgecolors = colors
+                # set the facecolor to 'none' (at the last chance) because
+                # we can not not fill a path if the facecolor is non-null.
+                # (which is defendable at the renderer level)
+                colors = 'none'
+            else:
+                # if we are not nulling the face color we can do this
+                # simpler
+                edgecolors = 'face'
+
             if linewidths is None:
                 linewidths = rcParams['lines.linewidth']
             elif np.iterable(linewidths):
@@ -4529,8 +4563,8 @@ def scatter(self, x, y, s=None, c=None, marker=None, cmap=None, norm=None,
 
         collection = mcoll.PathCollection(
                 (path,), scales,
-                facecolors=colors if marker_obj.is_filled() else 'none',
-                edgecolors=edgecolors if marker_obj.is_filled() else colors,
+                facecolors=colors,
+                edgecolors=edgecolors,
                 linewidths=linewidths,
                 offsets=offsets,
                 transOffset=kwargs.pop('transform', self.transData),
diff --git a/lib/matplotlib/tests/test_axes.py b/lib/matplotlib/tests/test_axes.py
@@ -2145,6 +2145,17 @@ def test_scatter_unfilled(self):
                                                    [0.5, 0.5, 0.5, 1]])
         assert_array_equal(coll.get_linewidths(), [1.1, 1.2, 1.3])
 
+    @pytest.mark.style('default')
+    def test_scatter_unfillable(self):
+        coll = plt.scatter([0, 1, 2], [1, 3, 2], c=['0.1', '0.3', '0.5'],
+                           marker='x',
+                           linewidths=[1.1, 1.2, 1.3])
+        assert_array_equal(coll.get_facecolors(), coll.get_edgecolors())
+        assert_array_equal(coll.get_edgecolors(), [[0.1, 0.1, 0.1, 1],
+                                                   [0.3, 0.3, 0.3, 1],
+                                                   [0.5, 0.5, 0.5, 1]])
+        assert_array_equal(coll.get_linewidths(), [1.1, 1.2, 1.3])
+
     def test_scatter_size_arg_size(self):
         x = np.arange(4)
         with pytest.raises(ValueError, match='same size as x and y'):
@@ -6939,3 +6950,13 @@ def test_patch_bounds():  # PR 19078
     bot = 1.9*np.sin(15*np.pi/180)**2
     np.testing.assert_array_almost_equal_nulp(
         np.array((-0.525, -(bot+0.05), 1.05, bot+0.1)), ax.dataLim.bounds, 16)
+
+
+@pytest.mark.style('default')
+def test_warn_ignored_scatter_kwargs():
+    with pytest.warns(UserWarning,
+                      match=r"You passed a edgecolor/edgecolors"):
+
+        c = plt.scatter(
+            [0], [0], marker="+", s=500, facecolor="r", edgecolor="b"
+        )
