Add appropriate error on color size mismatch in `scatter`

[bcongdon]

Addresses #7314.

I imagine that the call to ravel is a necessary evil, but if not, it might be more prudent to compare shape of x and c_array

[NelleV]

Can you please add a test?

[bcongdon]

Yup! Added one.

[NelleV]

Thanks!
LGTM

[efiring]

Scatter argument handling is tricky...

[efiring]

I don't think this is the right place or way to do the checking. It is not taking into account the docstring description of c. See below.

[efiring]

Note the comment. I think what you need to do is put in a check here to ensure that c is "acceptable as PathCollection facecolors". You can look at the Collections code to see what that entails.

[tacaswell]

Or push this checking in to collections and make sure a sensible exception is raised there?

[tacaswell]

Er, no we can't due to the masked point logic a few lines down.

[efiring]

In the interest of keeping tests minimal and direct, you could reduce N to 5, and use arange instead of rand.
You could also add a test for the handling of c when it is an rgba or sequence of them.

[efiring]

Looking back at @anntzer's comment in #7314, I think this line should be checking that c_array.shape matches the original shape of x and y. That implies that x and y should be checked earlier for matching shapes, and that their original common shape should be saved so it can be used in the c shape-check.

[tacaswell]

The docstrings say x/y must be (n, ) shaped, but it does not look like we check that anywhere. I suspect we have the ravel to in fact support (n, 1) and (1, n) shaped arrays arrays.

[story645]

What about a preprocess step where x and y get squeezed? that'd convert both (n, 1) and (1, n) to (n, ) and leave anything else alone.

[tacaswell]

I basically agree with @efiring on this review.

[bcongdon]

Thanks for the comments. I switched around the logic to test c_array against the original shape of x and y. I allowed it to match either one, as I wasn't sure how else to differentiate the (1,n) vs (n,1) case that @tacaswell described.

It looks like PathCollections eventually uses mcolors.to_rgba_array to check the validity of the facecolors, so I used that to assert the validity of c as a color sequence.

[tacaswell]

This error message should probably contain the x shape, y shape, and c shape. At this point we have tried all of the things the user could have meant, have not been able to work it out so we should give feedback on everything we tried.

[bcongdon]

Added the shape of c, x, and y to the ValueError message.

[story645]

@bcongdon I think the point is that arrays of shape (n, 1) and (1, n) should be treated as if they have equal shape, but checking on size is a bad idea 'cause an array of shape (2,2) has equal size with an array of (4,1) or (1,4).

[tacaswell]

@bcongdon This seems to have conflicts in the test file, can you rebase?

@efiring can you please re-review (you have a request changes which I am pretty sure are addressed).

I agree with @story645 that using squeeze instead of ravel would make more sense, but worry that would break undocumented (but probably relied on) behavior (because users). If we do that it should be a follow up PR and not block this one as-is.

[bcongdon]

Please excuse my novice git skills. Not sure if I rebased correctly... Is there a better way to rebase the current master changes without having all the subsequent commits showing up in the PR?

[tacaswell]

The last step needs to be a force-push. your last commit merges the just-rebased commits back into your local branch so they will show up twice. See http://matplotlib.org/devdocs/devel/gitwash/development_workflow.html?highlight=rebase#rebasing-a-pull-request-pr

[bcongdon]

Thanks! 👍

[QuLogic]

There are still 8 commits when there should be 4; when you rebase, be sure to drop the extra copy. Use git rebase -i for an interactive rebase and drop one of the copies.

[bcongdon]

Whoops, didn't realize that. Thanks

[efiring]

I see possibilities for improvement in this block: first, the error message is based on the raveled x and y shapes, not the original shapes, so this could be confusing. Maybe just say 'for x with size...' etc. and use x.size etc. After all, x and y are documented as 1-D and treated as 1-D, so it is their size that matters, not their shapes. Second, I think that you could make it both more compact and more efficient by keeping the work done by mcolors.to_rgba_array. You can delete the colors = c, and replace the line inside the 'try' with 'colors = mcolors.to_rgba_array(c)'. It will be subject to another to_rgba_array call inside PathCollection, which will be able to recognize that it already has an rgba array, instead of using a list comprehension with a function call for each color.

[bcongdon]

Sounds good. I made those changes.

[efiring]

When I tried to backport this, I realized that it contains an error: instead of adding a test, it is replacing an unrelated test. I should have noticed this before merging. I will fix it right now.

[efiring]

I don't want to spend any more time thrashing around with the tests, which are incompatible between v2.x and master, so I am not going to backport this. Therefore I am moving the milestone to 2.1. That should be soon enough.