Skip to content

Improve error reporting for scatter c as invalid RGBA. #14664

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

Closed
wants to merge 1 commit into from

Conversation

efiring
Copy link
Member

@efiring efiring commented Jul 1, 2019

Closes #11919.

PR Summary

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

@efiring efiring requested a review from anntzer July 1, 2019 00:45
@anntzer
Copy link
Contributor

anntzer commented Jul 1, 2019

I don't think this really improves the situation? With this PR

scatter([1], [2], c=np.array([[120,130,140]]))

gives

ValueError: 'c' argument must be a color, a sequence of colors, or a sequence of numbers, not [[120. 130. 140.]]

but this value of c is a sequence of number in the sense of scatter (which will flatten them): for example

scatter(np.array([1, 2, 3]), np.array([3, 4, 5]), c=np.array([[120,130,140]]))

works just fine.

Edit: edited incorrect example.

@efiring
Copy link
Member Author

efiring commented Jul 1, 2019

If you look at the docstring, in particular the note section, you will find that in the example in question, and in the test I added, c is not supposed to be flattened, and indeed it isn't. Therefore the change I made brings the error message into sync with the docstring and its intent, which is that if c is a 2-d array, and it's first dimension matches x.size, then it will be treated as a sequence of RGB or RGBA--or at least the attempt will be made. At that point, color mapping is out of the question, and the failure is not a color mapping size mismatch.

@anntzer
Copy link
Contributor

anntzer commented Jul 1, 2019

Right now the docstring reads

         c : color, sequence, or sequence of colors, optional
            The marker color. Possible values:

            - A single color format string.
            - A sequence of colors of length n.
            - A scalar or sequence of n numbers to be mapped to colors using
              *cmap* and *norm*.
            - A 2-D array in which the rows are RGB or RGBA.

but that's not really true: something like

scatter(range(6), range(6), c=np.array([[10, 20, 30], [40, 50, 60]]))

and there c is 2D and gets flattened.

Thus, if we take the docstring at its word, for the purposes of scatter, "sequence of n numbers" (to be mapped to colors) includes 2d-arrays that get flattened. I don't agree with this choice, but that's irrelevant here.

But then the error message "'c' argument must be a color, a sequence of colors, or a sequence of numbers, not [[120. 130. 140.]]" doesn't make sense, because [[120. 130. 140.]] is such a "sequence".

Or you need to fix third item of the docstring (perhaps by copying/lifting part of the notes section there).

@tacaswell tacaswell added this to the v3.2.0 milestone Jul 2, 2019
@efiring
Copy link
Member Author

efiring commented Jul 5, 2019

This isn't worth any more time.

@efiring efiring closed this Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Error Message
3 participants