Skip to content

Add "c" as alias for "color" for Collections #13454

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

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 17, 2019

... for consistency with Line2D, as well as the already existing "ec"
and "fc" aliases for "edgecolor" and "facecolor".

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

@timhoffm
Copy link
Member

Does this play nice with scatter(), which already has a c parameter?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 18, 2019

I guess scatter will just eat up the c argument by itself instead of passing it down to the alias machinery? Mentioned that in the changelog note.

... for consistency with Line2D, as well as the already existing "ec"
and "fc" aliases for "edgecolor" and "facecolor".
@timhoffm
Copy link
Member

My question is also: Did you check that the newly introduced API for whatever(c=...) is conceptually similar to scatter(c=...), e.g. in terms of accepted values, broadcasting, precedence color/facecolor etc.? It would be unfortunate to have functions with the same semantic and syntactic parameter c that would have an unnecessarily different API. It may be ok for some functions to have a narrower API, e.g. no broadcasting or not supporting multiple values (even though that should be synchronized).

@anntzer
Copy link
Contributor Author

anntzer commented Feb 18, 2019

fair point, needs checking

@anntzer
Copy link
Contributor Author

anntzer commented Feb 19, 2019

though likely #12021 will need to be resolved first to clean up the tangled mess

@timhoffm timhoffm added this to the v3.2.0 milestone Mar 3, 2019
@anntzer
Copy link
Contributor Author

anntzer commented Mar 25, 2019

Actually, it's clear that this cannot be consistent in the case of scatter because scatter's c argument can be colormapped whereas color cannot:

scatter([1, 2], [3, 4], color=[0, 1])

results in

ValueError: 'color' kwarg must be an mpl color spec or sequence of color specs.
For a sequence of values to be color-mapped, use the 'c' argument instead.

(the colormapped attribute is array, not color).

Any change would probably require more surgery, so I'll just close this for now.

@anntzer anntzer closed this Mar 25, 2019
@anntzer anntzer deleted the collectionalias branch March 25, 2019 22:03
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.

2 participants