Skip to content
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

make sure marker colors also accept np.array, fixes #8750 #8757

Merged
merged 1 commit into from Jun 23, 2017
Merged

make sure marker colors also accept np.array, fixes #8750 #8757

merged 1 commit into from Jun 23, 2017

Conversation

joergdietrich
Copy link
Contributor

PR Summary

Setting markerface/edgecolor with an np.array leads to a ValueError with numpy-1.13. See #8750 As suggested in the issue thread I replaced the comparison with np.array_equal(). I modified the unit test for marker styles to test a range of color specifications.

PR Checklist

  • [ x] Has Pytest style unit tests
  • [ x] Code is PEP 8 compliant
  • [ N/A] New features are documented, with examples if plot related
  • [ N/A] Documentation is sphinx and numpydoc compliant
  • [ N/A] Added an entry to doc/users/whats_new.rst if major new feature
  • [ N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

First PR to matplotlib. Hope this is fine.

@joergdietrich
Copy link
Contributor Author

Ah well, it seems np.array_equal doesn't work with numpy-1.7.1.

In [6]: equal('auto', 'k')
Out[6]: NotImplemented

Back to the drawing board.

@afvincent
Copy link
Contributor

afvincent commented Jun 13, 2017

Huh. It is weird because one can find these kind of functions in the 1.5 reference guide. See for example np.equal, np.array_equal or np.array_equiv.

Edit:
For the record

In [15]: np.__version__
Out[15]: '1.11.1'  # so I guess the issue is not Numpy 1.7 ^^

In [16]: np.equal('auto', 'k')
Out[16]: NotImplemented

In [17]: np.array_equal('auto', 'k')
Out[17]: False

In [18]: np.array_equiv('auto', 'k')
Out[18]: False

@tacaswell
Copy link
Member

Internally colors should be 4-tuples for actual colors or strings for 'special' values (like 'none', 'edge', or 'auto'). I think @anntzer made great progress on this actually being true. Instead of trying to run this through numpy, just checking isinstance(v, six.text_types) should be enough.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Jun 13, 2017
@joergdietrich
Copy link
Contributor Author

@afvincent Ok, so the problem is that in numpy-1.7.1 array_equal uses

return bool(equal(a1,a2).all())

which trips on NotImplemented but starting with 1.8 it is

return bool(asarray(a1 == a2).all())

@dstansby
Copy link
Member

How about if not np.all(self._markerfacecolor == fc) instead? (ie. if at least one markerfacecolor is differend to fc)

@dstansby
Copy link
Member

dstansby commented Jun 14, 2017

Or if np.any(self._markerfacecolor != fc) would be closer to the original and do the same

@joergdietrich
Copy link
Contributor Author

Thanks, @dstansby! Not sure what the failing codecov/patch check wants to tell me.

@dstansby
Copy link
Member

I'm not entirely sure why codecov/patch is failing either, all the changed lines seem to be tested. Thanks for the patch!

@anntzer
Copy link
Contributor

anntzer commented Jun 23, 2017

thanks!

@anntzer anntzer merged commit 3b0ee9f into matplotlib:master Jun 23, 2017
@joergdietrich joergdietrich deleted the array_equal branch June 23, 2017 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants