Skip to content

BUG: Limited available colors #38247

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
Merged
merged 6 commits into from
Dec 12, 2020
Merged

BUG: Limited available colors #38247

merged 6 commits into from
Dec 12, 2020

Conversation

ma3da
Copy link
Contributor

@ma3da ma3da commented Dec 2, 2020 •
edited
Loading

(GH37967)

@ma3da
Copy link
Contributor Author

ma3da commented Dec 2, 2020

Not sure if bug is the good label...

@ma3da
Copy link
Contributor Author

ma3da commented Dec 2, 2020

Two small scripts used for that.

from pandas.io.formats.excel import CSSToExcelConverter
from matplotlib.colors import CSS4_COLORS

# check if existing colors match the ones of matplotlib
for name, color in CSSToExcelConverter.NAMED_COLORS.items():
    assert "#" + color == CSS4_COLORS[name]

# generate code for CSSToExcelConverter
for name, color in CSS4_COLORS.items():
    print(f'"{name}": "{color[1:]}",')

ivanovmg
ivanovmg reviewed Dec 2, 2020
View reviewed changes
pandas/tests/io/formats/test_to_excel.py
Comment on lines +319 to +322
def tests_css_named_colors_valid():
upper_hexs = set(map(str.upper, string.hexdigits))
for color in CSSToExcelConverter.NAMED_COLORS.values():
assert len(color) == 6 and all(c in upper_hexs for c in color)
Copy link
Member

Choose a reason for hiding this comment

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

Would you consider adding a test that all colors from matplotlib.colors.CS4_COLORS are present in NAMED_COLORS?
Or would that be an overkill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wondered a bit about that, but didn't make my mind.
It's not long to write, so I've pushed a commit for that, but no problem for removing it.

jreback
jreback requested changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

-1 on adding a big list like this. let's just use the mpl machinery here (by importing the color defs)

pandas/io/formats/excel.py Outdated
@@ -65,27 +65,156 @@ class CSSToExcelConverter:
CSS processed by :meth:`__call__`.
"""

# source: matplotlib 3.3.3, matplotlib.colors.CSS4_COLORS
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just import these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possible of course, but is it worth introducing a dependency on mpl for excel manipulation?

Copy link
Member

@ivanovmg ivanovmg Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It sounds reasonable for me to have a list of colors, rather than depend on matplotlib.
Could think of placing the list into a separate module, so that pandas/io/formats/excel.py is not cluttered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used matplotlib nomenclature (_color_data.py), maybe something else will be more adequate

@jreback jreback added the Visualization plotting label Dec 3, 2020
@jreback jreback added this to the 1.2 milestone Dec 4, 2020
jreback
jreback requested changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

pls add a whatsnew note, other enhancements for 1.2

pandas/io/formats/_color_data.py
@@ -0,0 +1,151 @@
# source: matplotlib._color_data (3.3.3)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some more commentary here, why we need this and we don't want to depend on using mpl for this data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks. Do we have any need for this in plotting itself? @ivanovmg @charlesdong1991 @jorisvandenbossche

pandas/io/formats/excel.py Outdated
@@ -25,6 +25,8 @@
from pandas.io.formats.format import get_level_lengths
from pandas.io.formats.printing import pprint_thing

from ._color_data import CSS4_COLORS
Copy link
Contributor

Choose a reason for hiding this comment

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

use a fully qualifed import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jreback
Copy link
Contributor

jreback commented Dec 4, 2020

cc @charlesdong1991

@jorisvandenbossche
Copy link
Member

(removing 1.2 milestone, since this is not critical to include)

@jorisvandenbossche jorisvandenbossche removed this from the 1.2 milestone Dec 8, 2020
@jreback jreback added this to the 1.2 milestone Dec 12, 2020
@jreback jreback merged commit 36c4d5c into pandas-dev:master Dec 12, 2020
@jreback
Copy link
Contributor

jreback commented Dec 12, 2020

thanks @ma3da

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Dec 12, 2020
@ma3da @meeseeksmachine
Backport PR pandas-dev#38247: BUG: Limited available colors
2b2ca71
@meeseeksmachine meeseeksmachine mentioned this pull request Dec 12, 2020
Merged
jreback pushed a commit that referenced this pull request Dec 13, 2020
@meeseeksmachine @ma3da
Backport PR #38247: BUG: Limited available colors (#38435)
43e3fc3 
Co-authored-by: Luis Pinto <[email protected]>
luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
@ma3da @luckyvs1
BUG: Limited available colors (pandas-dev#38247)
6bb3459
@ma3da ma3da deleted the GH_37967_more_css_colors branch March 22, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanovmg ivanovmg ivanovmg left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Visualization plotting
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.

BUG: Limited available color name list when using style and to_excel
4 participants
@ma3da @jreback @jorisvandenbossche @ivanovmg