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.

Sign up for GitHub

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

Jump to bottom

BUG: Plotly scatter plots no longer produce color correctly, color alias for c in 1.5 #49732

Closed
3 tasks done
astrowonk opened this issue Nov 16, 2022 · 8 comments · Fixed by #49836
Closed
3 tasks done

BUG: Plotly scatter plots no longer produce color correctly, color alias for c in 1.5 #49732

astrowonk opened this issue Nov 16, 2022 · 8 comments · Fixed by #49836
Labels
Bug Regression Functionality that used to work in a prior pandas version Visualization plotting
Milestone
1.5.3

Comments

@astrowonk
Copy link

astrowonk commented Nov 16, 2022 •
edited
Loading

Pandas version checks

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of pandas.

  • I have confirmed this bug exists on the main branch of pandas.

Reproducible Example

import pandas as pd
import plotly.express as px
df = px.data.iris()

#Works
px.scatter(df, x="sepal_width", y="sepal_length", color="species")

#doesn't work as expected in 1.5.x
df.plot.scatter(x="sepal_width", y="sepal_length", color="species")

Issue Description

Plotly (and perhaps other backends) take a color argument that is a column name to use to make distinct colors for each variable (or even continuous color.) This is very convenient compared to the c variable in the pandas backend that takes a list of colors.

In Pandas 1.5, color and c in the back end code become the same, or rather color is an alias of c:

In addition, the code pops out the color argument so that it no longer exists in kwargs so the color information (expected to be a list of columns) is now absent. Changing the line to

color = kwargs.get('color')

would leave the variable in place, but then other plotting back ends would get color and presumably crash on an expected keyword, and would have to use c instead.

I think it would be best to not alias color to c for scatter plots and let backends like plotly continue to work as they did prior to 1.5.x

Expected Behavior

Plotly color kwarg should behave as expected prior to 1.5.x and pass through a column name to the plotly backend, not remove the kwarg and alias it to the c kwarg.

Installed Versions

/usr/local/opt/miniforge3/lib/python3.10/site-packages/_distutils_hack/init.py:33: UserWarning:

Setuptools is replacing distutils.

INSTALLED VERSIONS

commit : 91111fd
python : 3.10.6.final.0
python-bits : 64
OS : Darwin
OS-release : 22.1.0
Version : Darwin Kernel Version 22.1.0: Sun Oct 9 20:14:30 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T8103
machine : arm64
processor : arm
byteorder : little
LC_ALL : None
LANG : en_US.UTF-8
LOCALE : en_US.UTF-8

pandas : 1.5.1
numpy : 1.23.4
pytz : 2022.1
dateutil : 2.8.2
setuptools : 65.5.1
pip : 22.3.1
Cython : None
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : 3.1.2
IPython : 8.6.0
pandas_datareader: None
bs4 : 4.9.3
bottleneck : None
brotli :
fastparquet : None
fsspec : 2022.5.0
gcsfs : None
matplotlib : 3.5.3
numba : 0.56.3
numexpr : None
odfpy : None
openpyxl : 3.0.10
pandas_gbq : None
pyarrow : 9.0.0
pyreadstat : None
pyxlsb : None
s3fs : None
scipy : 1.9.0
snappy : None
sqlalchemy : 1.4.44
tables : None
tabulate : 0.8.10
xarray : None
xlrd : 2.0.1
xlwt : None
zstandard : None
tzdata : 2022.6
@astrowonk astrowonk added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Nov 16, 2022
@astrowonk astrowonk mentioned this issue Nov 16, 2022
Closed
@MarcoGorelli
Copy link
Member

That's a good point, thanks @astrowonk

It may be best to revert the PR that introduced the alias

@nicolaskruchten
Copy link

Thanks for reporting this @astrowonk and for looking into it @MarcoGorelli!

As the author of the plotly backend, I would definitely expect that the external API here be very stable... "removing" kwargs like this definitely breaks our backend, but so would adding special handling to previously-ignored kwargs. For example right now with the plotly backend, df.plot.scatter() accepts all of the kwargs that plotly.express.scatter() does, so any "upstream" modifications to the kwargs would break things: https://plotly.com/python-api-reference/generated/plotly.express.scatter.html

I had to add some code to adapt the kwarg lists for various df.plot methods here https://github.com/plotly/plotly.py/blob/master/packages/python/plotly/plotly/__init__.py#L99 (s, c, by etc) and I guess I could continue to do so but I sort of built this backend under the assumption that this stuff wouldn't change very much :) Maybe I misunderstood the contract between Pandas and plotting backends though!

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Nov 16, 2022 •
edited
Loading

Hey @nicolaskruchten

Maybe I misunderstood the contract between Pandas and plotting backends though!

No no, this is totally my fault for having approved https://github.com/pandas-dev/pandas/pull/44856/files . In fact, I should've rejected the issue instead of encouraging a PR. I wasn't thinking about the plotly backend because I don't use it (I use plotly directly by importing plotly and plotly.express)

Going forwards, something I've had on the back of my mind is to suggest to the rest of the pandas dev team is to declare pandas.plotting as being in "maintenance mode" only (my personal preference would be to remove most of it completely, as I think plotly.express and seaborn do the same thing but better. There's no comparison...). I'll open an issue about that soon.

Either way, you shouldn't need to worry about breaking changes in pandas.plotting.

Regarding what to do about this specific issue, I'll look into solutions more carefully later this week

@astrowonk
Copy link
Author

astrowonk commented Nov 16, 2022 •
edited
Loading

Going forwards, something I've had on the back of my mind is to suggest to the rest of the pandas dev team is to declare pandas.plotting as being in "maintenance mode" only (my personal preference would be to remove most of it completely, as I think plotly.express and seaborn do the same thing but better. There's no comparison...). I'll open an issue about that soon.

@MarcoGorelli I would just say that I have made extensive use of the pandas plotting back end syntax (albeit lately with plotly and the backend). I think it's important to keep so existing code keeps working with existing backends.

Were it to vanish, so many scripts and modules I have written would break, so whatever happens, please keep the df.plot syntax working!

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Nov 16, 2022 •
edited
Loading

Thanks @astrowonk , that's a useful perspective

I was thinking more about all the custom matplotlib code in pandas, as that's what causes the most issues - it could be greatly simplified, like with an entrypoint in seaborn similar to the one in plotly. I'll flesh that out in a separate issue anyway

@phofl phofl mentioned this issue Nov 16, 2022
Merged
5 tasks
@phofl phofl added this to the 1.5.3 milestone Nov 16, 2022
@phofl phofl added Visualization plotting Regression Functionality that used to work in a prior pandas version and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Nov 16, 2022
@rhshadrach
Copy link
Member

rhshadrach commented Nov 16, 2022 •
edited
Loading

declare pandas.plotting as being in "maintenance mode" only (my personal preference would be to remove most of it completely, as I think plotly.express and seaborn do the same thing but better.

I find it very useful in ad hoc data analysis to be able to do df['my_column'].plot.hist(bins=30) etc. I would be -1 on removal (wasn't entirely sure what "most of it completely" meant).

Edit:

I was thinking more about all the custom matplotlib code in pandas, as that's what causes the most issues - it could be greatly simplified, like with an entrypoint in seaborn similar to the one in plotly. I'll flesh that out in a separate issue anyway

Ah, I should have read further before posting. +1 here.

@nicolaskruchten
Copy link

Yeah my feeling is that the existing functionality can never go away... Maintenance mode sounds good to me though!

@MarcoGorelli MarcoGorelli mentioned this issue Nov 22, 2022
Merged
5 tasks
@MarcoGorelli
Copy link
Member

I was thinking more about all the custom matplotlib code in pandas, as that's what causes the most issues - it could be greatly simplified, like with an entrypoint in seaborn similar to the one in plotly. I'll flesh that out in a separate issue anyway

Finally got round to it - here's my proposal: #50059

@MarcoGorelli MarcoGorelli mentioned this issue Dec 5, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Regression Functionality that used to work in a prior pandas version Visualization plotting
Projects
None yet
Milestone
1.5.3
Development

Successfully merging a pull request may close this issue.

DEPR Enforce reversion of Size color aliases MarcoGorelli/pandas
5 participants
@nicolaskruchten @astrowonk @MarcoGorelli @rhshadrach @phofl