Skip to content

Deprecate color keyword argument in scatter #4675

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 3 commits into from

Conversation

jhamrick
Copy link
Contributor

There is a comment in Axes.scatter stating "'color' should be deprecated in scatter, or clearly defined; since it isn't, I am giving it low priority.". It is neither defined in the documentation nor deprecated, so this adds a deprecated warning to be more explicit about it.

@jenshnielsen
Copy link
Member

👍

@jhamrick
Copy link
Contributor Author

Also removed uses of the color keyword argument in examples, replacing it with c.

@tacaswell
Copy link
Member

Can you add a note in api_changes?

@tacaswell tacaswell added this to the next point release milestone Jul 13, 2015
@jhamrick
Copy link
Contributor Author

Done.

@tacaswell
Copy link
Member

Where the color kwargs comes from in Artist all the way at the bottom of the call stack (any kwargs which make it to the Artist.__init__ method are used to search for self.set_* methods which are then called with the value. In that sense we can't really deprecate the use of color as a kwargs.

@tacaswell tacaswell modified the milestones: next point release, proposed next point release Jul 14, 2015
@jenshnielsen
Copy link
Member

@tacaswell Then perhaps just change the warning to a different type and inform the user that it is discouraged?

@tacaswell
Copy link
Member

Hopefully this is something that traitlets will help identify and clean up.

@pelson
Copy link
Member

pelson commented Sep 25, 2015

The c keyword argument is grim. The number of times that I've been asked "what do 's' and 'c' do?"...

Maybe we are looking at this the wrong way?

@mwaskom
Copy link

mwaskom commented Nov 10, 2015

If possible, I'd like to ask that you please not remove color from scatter. Doing would disrupt higher level functions that are written against a general API and assume that passing color to a lower-level matplotlib function will do something useful (i.e., FacetGrid in seaborn). I also agree that from a user perspective, it would be confusing for color to work in many functions but not in scatter.

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Sep 24, 2017
@jklymak jklymak modified the milestones: needs sorting, v3.0 May 9, 2018
@tacaswell tacaswell modified the milestones: v3.0, v3.1 Jul 9, 2018
@jklymak
Copy link
Member

jklymak commented Aug 17, 2018

Does anyone want to resurrect this? I suspect its still a problem. Note that the deprecation should use our deprecation function....

@jklymak jklymak added Good first issue Open a pull request against these issues if there are no active ones! and removed status: needs review labels Aug 17, 2018
@mwaskom
Copy link

mwaskom commented Aug 17, 2018

I guess I’d repeat my request that matplotlib not act to make the API less consistent.

@jklymak
Copy link
Member

jklymak commented Aug 17, 2018

I think the issue is that color is ambiguous, whereas facecolor and edgecolor are not. If we are to keep color, then I think we need to agree on a meaning of it across objects that have both face/edgecolor. I don't particuarly care myself - I'd say c/color is what you start with for both, and face/edgecolor override. If thats whats already done, then maybe this should just be closed, or reduced to documenting color.

@mwaskom
Copy link

mwaskom commented Aug 17, 2018

I think the issue is that color is ambiguous, whereas facecolor and edgecolor are not. If we are to keep color, then I think we need to agree on a meaning of it across objects that have both face/edgecolor.

I don't believe that's the issue here. The color vs facecolor/edgecolor distinction is present in several matplotlib functions and should not be handled only by deprecating color in scatter. Were we to discuss that issue I would also stress the benefits of having all matplotlib functions accept and use color. But the issue here is about the specific confusion of having c and color. While I appreciate how having multiple ways to specify the color is confusing, I don't think the best solution is the remove the way that works everywhere else in matplotlib.

@jklymak
Copy link
Member

jklymak commented Aug 17, 2018

Let me rephrase. The fundamental issue is that color is ambiguous. I tend to agree with you that removing it is not correct. However, what to do with it is the question, and how to fix the documentation?

@mwaskom
Copy link

mwaskom commented Aug 17, 2018

color changes the color of the could of points uniformly. c changes the color of each point independently. You can also provide a single color to c and it will "broadcast" to all the points.

@ImportanceOfBeingErnest
Copy link
Member

So would it be possible to make c an alias of color (or vice versa); and possily the same with s and size? Meaning you can specify either of them (but not both) and see the same plot.
I suppose there can't be any use case where both are needed simultaneously. The only issue may be that color accepts an rbg color like color=[0.,0.,1.] while c may not (at least in the case you have 3 scatter points. Or are there other differences?

@mwaskom
Copy link

mwaskom commented Aug 17, 2018

The behavior you describe already exists:

plt.scatter([0, 1], [0, 1], c="red", color="blue")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-31-7bf4d623a32e> in <module>()
----> 1 plt.scatter([0, 1], [0, 1], c="red", color="blue")

~/anaconda/lib/python3.6/site-packages/matplotlib/pyplot.py in scatter(x, y, s, c, marker, cmap, norm, vmin, vmax, alpha, linewidths, verts, edgecolors, hold, data, **kwargs)
   3468                          vmin=vmin, vmax=vmax, alpha=alpha,
   3469                          linewidths=linewidths, verts=verts,
-> 3470                          edgecolors=edgecolors, data=data, **kwargs)
   3471     finally:
   3472         ax._hold = washold

~/anaconda/lib/python3.6/site-packages/matplotlib/__init__.py in inner(ax, *args, **kwargs)
   1853                         "the Matplotlib list!)" % (label_namer, func.__name__),
   1854                         RuntimeWarning, stacklevel=2)
-> 1855             return func(ax, *args, **kwargs)
   1856 
   1857         inner.__doc__ = _add_data_doc(inner.__doc__,

~/anaconda/lib/python3.6/site-packages/matplotlib/axes/_axes.py in scatter(self, x, y, s, c, marker, cmap, norm, vmin, vmax, alpha, linewidths, verts, edgecolors, **kwargs)
   4213                 facecolors = co
   4214             if c is not None:
-> 4215                 raise ValueError("Supply a 'c' kwarg or a 'color' kwarg"
   4216                                  " but not both; they differ but"
   4217                                  " their functionalities overlap.")

ValueError: Supply a 'c' kwarg or a 'color' kwarg but not both; they differ but their functionalities overlap.

Although that message could probably be a bit more specific...

@ImportanceOfBeingErnest
Copy link
Member

Exactly. "they differ but their functionalities overlap".
I'm asking to not let them differ (=alias), or at least work towards not letting them differ if that breaks existing functionality.

@mwaskom
Copy link

mwaskom commented Aug 17, 2018

Feel free to pursue that; I will disengage from the conversation.

I think that it's nice to be able to pass an RGB tuple to color and have it do the right thing even with 3 points — the change to how c behaves in that circumstance caused me and others a lot of headaches.

My only aim here has been to strongly encourage matplotlib not to remove the ability to pass a normal color specification to color= and I think I've made that point as well as I can.

@tacaswell tacaswell added API: changes and removed Good first issue Open a pull request against these issues if there are no active ones! labels Aug 17, 2018
@tacaswell
Copy link
Member

As a rule I think api changes and deprecation are not good first issues.

@jklymak
Copy link
Member

jklymak commented Oct 6, 2018

OK I'm closing this because it doesn't really seem to be an urgent issue, and maybe we need to figure out library wide how to handle color, edgecolor, facecolor. Feel free to re-open.

@jklymak jklymak closed this Oct 6, 2018
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.

7 participants