Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Scatter: make "c" and "s" argument handling more consistent.#13959
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| c =np.ma.ravel(c_array) | ||
| n_elem = c_array.size | ||
| ifn_elem == xsize: | ||
| c =c_array.ravel() |
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.
did we need this cast to masked here?
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 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.
lib/matplotlib/axes/_axes.py Outdated
| if c_array.shape in [xshape, yshape]: | ||
| c = np.ma.ravel(c_array) | ||
| n_elem = c_array.size | ||
| if n_elem == xsize: |
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.
why do we definen_elem here? its a confusing name now that its a size, and I don't see it adding anything.
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 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.
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 extracted the code fromscatter 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 namedcsize in resemblence toxsize,ysize.
lib/matplotlib/axes/_axes.py Outdated
| @@ -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] | |||
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.
then we define n_elem here, and don't use in the next line...
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.
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.
Not sure if this is the right way to go. This makes |
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 |
lib/matplotlib/axes/_axes.py Outdated
| if c_array.shape in [xshape, yshape]: | ||
| c = np.ma.ravel(c_array) | ||
| n_elem = c_array.size | ||
| if n_elem == xsize: |
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 extracted the code fromscatter 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 namedcsize in resemblence toxsize,ysize.
lib/matplotlib/axes/_axes.py Outdated
| # NB: remember that a single color is also acceptable. | ||
| # Besides *colors* will be an empty array if c == 'none'. | ||
| valid_shape = False | ||
| raise ValueError | ||
| except ValueError: | ||
| except(ValueError, TypeError): |
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.
where does the typeerror come from?
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.
In [9]:mcolors.to_rgba_array(None)---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast)<ipython-input-9-1ff544d60239>in<module>---->1mcolors.to_rgba_array(None)~/work/programs/py/mpl/matplotlib/lib/matplotlib/colors.pyinto_rgba_array(c,alpha)282pass283# Convert one at a time.-->284result=np.empty((len(c),4),float)285fori,ccinenumerate(c):286result[i]=to_rgba(cc,alpha)TypeError:objectof type'NoneType'hasnolen()
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.
Not to be a pain... but canc 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?
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.
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.
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.
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.
There seems to be a new but unrelated failure in the docs build (involving the hline docstring) after my last commit. |
lib/matplotlib/axes/_axes.py Outdated
| "'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) |
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.
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)
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.
done
As a heads up:#14113 |
Uh oh!
There was an error while loading.Please reload this page.
Closes#12735.
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