NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Fix scatter() with empty data and c argument#12672

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

Jump to bottom

Closed

timhoffm wants to merge1 commit intomatplotlib:masterfromtimhoffm:fix-scatter-empty-color

Closed

Fix scatter() with empty data and c argument#12672

timhoffm wants to merge1 commit intomatplotlib:masterfromtimhoffm:fix-scatter-empty-color

Conversation

Copy link

Member

timhoffm commentedOct 29, 2018

PR Summary

Fixes#12641. The issue is thatc[0] is accessed for an empty array (the 3d code drops invalid points and thus passes an empty array in case of the MWE#12641 (comment)).

Note: While the canonical way of checking emptiness isif c, I feel it's more clear to uselen(c) > 0 here.

PR Checklist

Has Pytest style unit tests

Fix scatter() with empty data and c argument

70bbe79

tacaswell added this to thev3.0.x milestone

Oct 29, 2018

Copy link

Member

tacaswell commentedOct 29, 2018

if c will fail on numpy arrays with "Truth value of an array is ambigious, use .any or .all" ;)

Can you also usesafe_first_element instead ofc[0]?

tacaswell reviewed

Oct 29, 2018

View reviewed changes

lib/matplotlib/tests/test_axes.py

		@@ -1741,6 +1741,11 @@ def test_scatter_color(self):
		with pytest.raises(ValueError):
		plt.scatter([1, 2, 3], [1, 2, 3], color=[1, 2, 3])

		def test_scatter_empty(self):

Copy link

Member

tacaswellOct 29, 2018

Choose a reason for hiding this comment

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

Can you also add a test for pandas with a non-0 based index?

ImportanceOfBeingErnest mentioned this pull request

Oct 30, 2018

Fix for _axes.scatter() array index out of bound error#12673

Merged

Copy link

Member

ImportanceOfBeingErnest commentedOct 30, 2018

There is also#12673 with the same aim to fix this.

Copy link

Member

jklymak commentedOct 31, 2018

I'm going to close this@TimHoffman, in favour of#12673, because the proposer for that PR put a lot of effort into getting consensus before opening their PR. Hope that's OK....