Skip to content

make Axes._parse_scatter_color_args static #12739

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.

Sign up for GitHub

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
Nov 24, 2018
Merged

make Axes._parse_scatter_color_args static #12739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions lib/matplotlib/axes/_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4013,7 +4013,9 @@ def dopatch(xs, ys, **kwargs):
return dict(whiskers=whiskers, caps=caps, boxes=boxes,
medians=medians, fliers=fliers, means=means)

def _parse_scatter_color_args(self, c, edgecolors, kwargs, xshape, yshape):
@staticmethod
def _parse_scatter_color_args(c, edgecolors, kwargs, xshape, yshape,
get_next_color_func):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be an optional kwarg to save the mysterious kwarg in the tests?

If the kwarg stays in the tests, can you comment that its a dummy function...

We had someone be pretty firm that the tests should be of user-facing features, and not testing internal methods. I'm not sure I 100% buy that idea, but it is something to consider here. Does testing ax.scatter directly tests that no-one messes up the parsing before or after this fcn?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be an optional kwarg to save the mysterious kwarg in the tests?

The only usage of this function is to extract the parameter parsing, and in that call the argument is mandatory. I don't want to modify the signature to make the impression that a general use is possible without this parameter. The tests don't use it because we control inputs and thus the code path, but that's rather a consequence of the things we want to test and shouldn't affect the signature.

We had someone be pretty firm that the tests should be of user-facing features, and not testing internal methods. I'm not sure I 100% buy that idea, but it is something to consider here. Does testing ax.scatter directly tests that no-one messes up the parsing before or after this fcn?

Generally I agree that you should only test the public interface. However, this is really expensive here, because you would have to generate a plot for each parameter. Since the color parameters are quite involved, they need a lot of testing. So we're splitting the function into two parts: 1. parse the color arguments to facecolors and edgecolors, 2. draw with these colors.

Part 1 can be tested separately and efficiently. It's still implicitly part of the public API. If something goes wrong in here, the plot will be broken. Part 2 needs testing as well to make sure no-one messes up the parsing before or after part 1. But that can happen with just a few simple tests. Also this should test the actual plotting.

The modified tests were only a subset of part 1 anyway. They did only smoke-test if the parameters were accepted or not.

"""
Helper function to process color related arguments of `.Axes.scatter`.

Expand All @@ -4023,7 +4025,7 @@ def _parse_scatter_color_args(self, c, edgecolors, kwargs, xshape, yshape):
- kwargs['facecolors']
- kwargs['facecolor']
- kwargs['color'] (==kwcolor)
- 'b' if in classic mode else next color from color cycle
- 'b' if in classic mode else the result of ``get_next_color_func()``

Argument precedence for edgecolors:

Expand All @@ -4044,6 +4046,16 @@ def _parse_scatter_color_args(self, c, edgecolors, kwargs, xshape, yshape):
Note: The dict is modified by this function.
xshape, yshape : tuple of int
The shape of the x and y arrays passed to `.Axes.scatter`.
get_next_color_func : callable
A callable that returns a color. This color is used as facecolor
if no other color is provided.

Note, that this is a function rather than a fixed color value to
support conditional evaluation of the next color. As of the
current implementation obtaining the next color from the
property cycle advances the cycle. This must only happen if we
actually use the color, which will only be decided within this
method.

Returns
-------
Expand Down Expand Up @@ -4090,7 +4102,7 @@ def _parse_scatter_color_args(self, c, edgecolors, kwargs, xshape, yshape):
if c is None:
c = (facecolors if facecolors is not None
else "b" if rcParams['_internal.classic_mode']
else self._get_patches_for_fill.get_next_color())
else get_next_color_func())

# After this block, c_array will be None unless
# c is an array for mapping. The potential ambiguity
Expand Down Expand Up @@ -4289,8 +4301,9 @@ def scatter(self, x, y, s=None, c=None, marker=None, cmap=None, norm=None,
s = np.ma.ravel(s) # This doesn't have to match x, y in size.

c, colors, edgecolors = \
self._parse_scatter_color_args(c, edgecolors, kwargs,
xshape, yshape)
self._parse_scatter_color_args(
c, edgecolors, kwargs, xshape, yshape,
get_next_color_func=self._get_patches_for_fill.get_next_color)

# `delete_masked_points` only modifies arguments of the same length as
# `x`.
Expand Down
39 changes: 26 additions & 13 deletions lib/matplotlib/tests/test_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1793,19 +1793,30 @@ def test_scatter_color(self):

@pytest.mark.parametrize('c_case, re_key', params_test_scatter_c)
def test_scatter_c(self, c_case, re_key):
def get_next_color():
return 'blue' # currently unused

from matplotlib.axes import Axes

xshape = yshape = (4,)

# Additional checking of *c* (introduced in #11383).
REGEXP = {
"shape": "^'c' argument has [0-9]+ elements", # shape mismatch
"conversion": "^'c' argument must be a mpl color", # bad vals
}
x = y = [0, 1, 2, 3]
fig, ax = plt.subplots()

if re_key is None:
ax.scatter(x, y, c=c_case, edgecolors="black")
Axes._parse_scatter_color_args(
c=c_case, edgecolors="black", kwargs={},
xshape=xshape, yshape=yshape,
get_next_color_func=get_next_color)
else:
with pytest.raises(ValueError, match=REGEXP[re_key]):
ax.scatter(x, y, c=c_case, edgecolors="black")
Axes._parse_scatter_color_args(
c=c_case, edgecolors="black", kwargs={},
xshape=xshape, yshape=yshape,
get_next_color_func=get_next_color)


def _params(c=None, xshape=(2,), yshape=(2,), **kwargs):
Expand All @@ -1829,11 +1840,12 @@ def _params(c=None, xshape=(2,), yshape=(2,), **kwargs):
_result(c=['b', 'g'], colors=np.array([[0, 0, 1, 1], [0, .5, 0, 1]]))),
])
def test_parse_scatter_color_args(params, expected_result):
def get_next_color():
return 'blue' # currently unused

from matplotlib.axes import Axes
dummyself = 'UNUSED' # self is only used in one case, which we do not
# test. Therefore we can get away without costly
# creating an Axes instance.
c, colors, _edgecolors = Axes._parse_scatter_color_args(dummyself, *params)
c, colors, _edgecolors = Axes._parse_scatter_color_args(
*params, get_next_color_func=get_next_color)
assert c == expected_result.c
assert_allclose(colors, expected_result.colors)

Expand All @@ -1855,15 +1867,16 @@ def test_parse_scatter_color_args(params, expected_result):
(dict(color='r', edgecolor='g'), 'g'),
])
def test_parse_scatter_color_args_edgecolors(kwargs, expected_edgecolors):
def get_next_color():
return 'blue' # currently unused

from matplotlib.axes import Axes
dummyself = 'UNUSED' # self is only used in one case, which we do not
# test. Therefore we can get away without costly
# creating an Axes instance.
c = kwargs.pop('c', None)
edgecolors = kwargs.pop('edgecolors', None)
_, _, result_edgecolors = \
Axes._parse_scatter_color_args(dummyself, c, edgecolors, kwargs,
xshape=(2,), yshape=(2,))
Axes._parse_scatter_color_args(c, edgecolors, kwargs,
xshape=(2,), yshape=(2,),
get_next_color_func=get_next_color)
assert result_edgecolors == expected_edgecolors


Expand Down