Scatter: make "c" and "s" argument handling more consistent. by efiring · Pull Request #13959 · matplotlib/matplotlib

efiring · 2019-04-14T23:44:41Z

PR Summary

This is a small modification to #11663. It bases all checks on the sizes rather than the shapes of x, y, and c, consistent with the practice of flattening x and y at an early stage. This simplifies the code and reduces surprises such as those described in #10365 and #12735.

I also added a check that s be a scalar, or that its size match that of x and y. I think this at least partially addresses #12021, and makes the behavior consistent with the docs.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Closes matplotlib#12735.

jklymak · 2019-04-16T04:53:13Z

-                    c = np.ma.ravel(c_array)
+                n_elem = c_array.size
+                if n_elem == xsize:
+                    c = c_array.ravel()


did we need this cast to masked here?

I don't think so. The method will work regardless, since c_array is some kind of array, and I don't think we are later depending on c being a masked array.

jklymak · 2019-04-16T04:53:36Z

-                if c_array.shape in [xshape, yshape]:
-                    c = np.ma.ravel(c_array)
+                n_elem = c_array.size
+                if n_elem == xsize:


why do we define n_elem here? its a confusing name now that its a size, and I don't see it adding anything.

I didn't change this--n_elem is being used the same way it was before this PR. Throughout, n_elem is the number of colors, regardless of whether they are coming from a mapping or from an rgba array. It is used only in reporting an error, if one occurs.

I extracted the code from scatter to make it more manageable. It's may not always be the best code style, but I didn't change too much to keep the extraction easier to review.

This should now be named csize in resemblence to xsize, ysize.

jklymak · 2019-04-16T04:54:54Z

@@ -4263,18 +4260,17 @@ def _parse_scatter_color_args(c, edgecolors, kwargs, xshape, yshape,
            try:  # Then is 'c' acceptable as PathCollection facecolors?
                colors = mcolors.to_rgba_array(c)
                n_elem = colors.shape[0]


then we define n_elem here, and don't use in the next line...

Again, this is the way it was, and as far as I can see, it still works as intended, providing information for an error message.

timhoffm · 2019-04-17T21:40:10Z

Not sure if this is the right way to go. This makes c and s more compatible with the auto-flatting of x, y.
However, it's at least debatable if auto-flatting is desirable #12735 (comment).

efiring · 2019-04-17T23:25:40Z

If you want to remove auto-flattening, that's a big change for the users, and a different PR. What would it gain in the code? As far as I can see, it would have very little effect on the total LOC, including the docstring. The problem with c is not the flattening, it is all the different ways it can be specified and can interact with marker color kwargs.

timhoffm · 2019-04-26T12:47:25Z

-                if c_array.shape in [xshape, yshape]:
-                    c = np.ma.ravel(c_array)
+                n_elem = c_array.size
+                if n_elem == xsize:


I extracted the code from scatter to make it more manageable. It's may not always be the best code style, but I didn't change too much to keep the extraction easier to review.

This should now be named csize in resemblence to xsize, ysize.

anntzer · 2019-04-30T08:01:57Z

                    valid_shape = False
                    raise ValueError
-            except ValueError:
+            except (ValueError, TypeError):


where does the typeerror come from?

In [9]: mcolors.to_rgba_array(None) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-9-1ff544d60239> in <module> ----> 1 mcolors.to_rgba_array(None) ~/work/programs/py/mpl/matplotlib/lib/matplotlib/colors.py in to_rgba_array(c, alpha) 282 pass 283 # Convert one at a time. --> 284 result = np.empty((len(c), 4), float) 285 for i, cc in enumerate(c): 286 result[i] = to_rgba(cc, alpha) TypeError: object of type 'NoneType' has no len()

Not to be a pain... but can c really be None at that point? If so, why didn't the previous version need to handle that, or what example (what scatter() call) would cause an uncaught exception in the previous version?

It's a reasonable question. I added the TypeError because a test was otherwise failing, if I remember correctly. (I can't imagine any other reason I would have put it in.) I think it was because of a None. I will have to take out the TypeError and re-run the tests to find out.

Trying it now without catching the TypeError, tests pass on my machine, so maybe it was needed only at an intermediate stage while I was working on the PR. I've reverted that part of the change. Let's see if the tests still pass here, apart from the unrelated docs problem.

efiring · 2019-04-30T17:42:12Z

There seems to be a new but unrelated failure in the docs build (involving the hline docstring) after my last commit.

anntzer · 2019-05-01T19:25:26Z

-                        "'y' with size {ys}."
-                            .format(nc=n_elem, xs=xsize, ys=ysize)
+                        "acceptable for use with 'x' and 'y' with size {xs}."
+                            .format(nc=csize, xs=xsize)


just make this a f-string, or at least use the same names in the braces and outside (yes, I know, the problem was already there before)

timhoffm · 2019-05-01T20:04:01Z

As a heads up: #14113

efiring added 2 commits April 14, 2019 13:30

Scatter: make "c" argument handling more consistent.

5a5c2f6

Closes matplotlib#12735.

Require 's' arg to be a scalar or match 'x' in size.

198b3f1

efiring changed the title ~~Scatter: make "c" argument handling more consistent.~~ Scatter: make "c" and "s" argument handling more consistent. Apr 15, 2019

jklymak reviewed Apr 16, 2019

View reviewed changes

efiring mentioned this pull request Apr 17, 2019

Inconsistent shape handling of parameter c compared to x/y in scatter() #12735

Closed

timhoffm approved these changes Apr 26, 2019

View reviewed changes

anntzer reviewed Apr 30, 2019

View reviewed changes

More consistent naming: n_elem -> csize

290c9c0

Revert catching of TypeError along with ValueError

d7db876

anntzer reviewed May 1, 2019

View reviewed changes

Simplify error messages via f-strings.

c1add44

anntzer approved these changes May 1, 2019

View reviewed changes

anntzer merged commit e3c9376 into matplotlib:master May 1, 2019

efiring deleted the scatter_ravel_consistency branch May 1, 2019 21:06

anntzer mentioned this pull request Jun 24, 2019

Fix wrong error message in #11919 #11930

Closed

6 tasks

QuLogic added this to the v3.2.0 milestone Jul 24, 2019

anntzer mentioned this pull request Sep 10, 2019

scatter() behavior with different size inputs depends on the backend but both variants are inconsistent with the docs #12021

Closed

QuLogic mentioned this pull request May 15, 2020

Scatter produce multiple colors for a single RGB/RGBA input #17423

Closed

Uh oh!

Conversation

efiring commented Apr 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm commented Apr 17, 2019

Uh oh!

efiring commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiring commented Apr 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffm commented May 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

efiring commented Apr 14, 2019 •

edited

Loading