Skip to content

Color text based on background color when using _background_gradient() #21263

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

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 48 additions & 9 deletions pandas/io/formats/style.py
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ def highlight_null(self, null_color='red'):
return self

def background_gradient(self, cmap='PuBu', low=0, high=0, axis=0,
subset=None):
subset=None, text_color_threshold=0.408):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a Raises section for the docstring?

Color the background in a gradient according to
the data in each column (optionally row).
Expand All @@ -879,26 +879,34 @@ def background_gradient(self, cmap='PuBu', low=0, high=0, axis=0,
1 or 'columns' for columnwise, 0 or 'index' for rowwise
subset: IndexSlice
a valid slice for ``data`` to limit the style application to
text_color_threshold: float or int
Copy link
Contributor

Choose a reason for hiding this comment

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

Add , default 0.408.

And may a note as to why that's the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We chose 0.408 to stay consistent with the Seaborn implementation. Should I ask the Seaborn author for the underlying reason, just reference seaborn, or leave the value without explanation?

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, was just curious.

luminance threshold for determining text color. Facilitates text
visibility across varying background colors. From 0 to 1.
0 = all text is dark colored, 1 = all text is light colored.
Copy link
Member

Choose a reason for hiding this comment

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

Can add a version added for 0.24.0 for this

Copy link
Contributor

Choose a reason for hiding this comment

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

is this the common name for this field in mpl?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO text_color=0.2 looks very counterintuitive to me. It almost looks like the exact opposite of what this feature is about (not having a constant text color).
Shouldn't the name at least contain "threshold", e.g. text_color_threshold?

Copy link
Contributor Author

@joelostblom joelostblom May 31, 2018

Choose a reason for hiding this comment

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

I agree that text_color_threshold is suitable, is it ok with multiple underscores in the parameter name? I don't think there is a common name for this in mpl (for coloring text in general, they tend to use just color, but I believe it would be easy to mistake that for the backgruond color here due to the method's name)


.. versionadded:: 0.24.0

Returns
-------
self : Styler

Notes
-----
Tune ``low`` and ``high`` to keep the text legible by
not using the entire range of the color map. These extend
the range of the data by ``low * (x.max() - x.min())``
and ``high * (x.max() - x.min())`` before normalizing.
Set ``text_color_threshold`` or tune ``low`` and ``high`` to keep the
text legible by not using the entire range of the color map. The range
of the data is extended by ``low * (x.max() - x.min())`` and ``high *
(x.max() - x.min())`` before normalizing.
"""
subset = _maybe_numeric_slice(self.data, subset)
subset = _non_reducing_slice(subset)
self.apply(self._background_gradient, cmap=cmap, subset=subset,
axis=axis, low=low, high=high)
axis=axis, low=low, high=high,
text_color_threshold=text_color_threshold)
return self

@staticmethod
def _background_gradient(s, cmap='PuBu', low=0, high=0):
def _background_gradient(s, cmap='PuBu', low=0, high=0,
text_color_threshold=0.408):
"""Color background in a range according to the data."""
with _mpl(Styler.background_gradient) as (plt, colors):
rng = s.max() - s.min()
Expand All @@ -909,8 +917,39 @@ def _background_gradient(s, cmap='PuBu', low=0, high=0):
# https://github.com/matplotlib/matplotlib/issues/5427
normed = norm(s.values)
c = [colors.rgb2hex(x) for x in plt.cm.get_cmap(cmap)(normed)]
return ['background-color: {color}'.format(color=color)
for color in c]
if (not isinstance(text_color_threshold, (float, int)) or
not 0 <= text_color_threshold <= 1):
msg = "`text_color_threshold` must be a value from 0 to 1."
raise ValueError(msg)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test to ensure this raises?

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 am having troubles getting the test correct for this. When I try the function manually, it raises ValueError when called with any one of the parameters in the test, but pytest keeps failing saying that it doesn't raise a ValueError. Would you have time to check my latest commits and advise?


def relative_luminance(color):
"""
Calculate relative luminance of a color.

The calculation adheres to the W3C standards
(https://www.w3.org/WAI/GL/wiki/Relative_luminance)

Parameters
----------
color : matplotlib color
Hex code, rgb-tuple, or HTML color name.

Returns
-------
float
The relative luminance as a value from 0 to 1
"""
rgb = colors.colorConverter.to_rgba_array(color)[:, :3]
rgb = np.where(rgb <= .03928, rgb / 12.92,
((rgb + .055) / 1.055) ** 2.4)
lum = rgb.dot([.2126, .7152, .0722])
return lum.item()

text_colors = ['#f1f1f1' if relative_luminance(x) <
text_color_threshold else '#000000' for x in c]

return ['background-color: {color};color: {tc}'.format(
color=color, tc=tc) for color, tc in zip(c, text_colors)]

def set_properties(self, subset=None, **kwargs):
"""
Expand Down
17 changes: 16 additions & 1 deletion pandas/tests/io/formats/test_style.py
Original file line number Diff line number Diff line change
Expand Up @@ -1028,10 +1028,25 @@ def test_background_gradient(self):
assert all("#" in x[0] for x in result.values())
assert result[(0, 0)] == result[(0, 1)]
assert result[(1, 0)] == result[(1, 1)]
for res in result:
Copy link
Member

Choose a reason for hiding this comment

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

Make this a separate test called test_text_color_threshold to distinguish it from the gradient testing.

Also I didn't really understand the point of the conditional - can we not be more assertive about the exact values we'd expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking is that we want to test if the text color is light or dark conditional on the background color. If for some unexpected reason the background color changes, this test should not fail.

I added a section to assert if the background color takes on one of its 4 expected values to prevent that test_text_color_threshold doesn't test anything (I could move this section to test_background_gradient if you think that is more suitable).

if result[res][0].split(' ')[1] in ['#fde725', '#ffffcc']:
assert result[(res)][1].split(' ')[1] == '#000000'
elif result[res][0].split(' ')[1] in ['#800026', '#440154']:
assert result[(res)][1].split(' ')[1] == '#f1f1f1'

result = df.style.background_gradient(
subset=pd.IndexSlice[1, 'A'])._compute().ctx
assert result[(1, 0)] == ['background-color: #fff7fb']

assert result[(1, 0)] == ['background-color: #fff7fb',
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a more comprehensive / dedicated test for this? Something that encompasses the full range of expected values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a suggestion

'color: #000000']

@td.skip_if_no_mpl
Copy link
Member

Choose a reason for hiding this comment

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

Do these tests need to be in the MatplotlibDep test class? I didn't think this required mpl but could be wrong.

If that is the case then move the @td.skip_if_no_mpl decorator to the class instead of on each function individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they both call background_gradient() which depends on mpl.

def test_text_color_threshold(self):
Copy link
Member

Choose a reason for hiding this comment

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

Rename this to test_text_color_threshold_raises

df = pd.DataFrame([[1, 2], [2, 4]], columns=['A', 'B'])
for text_color_threshold in [1.1, '1', -1, [2, 2]]:
Copy link
Member

Choose a reason for hiding this comment

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

You can parametrize these (check test_to_html.py for examples if you don't know what I mean)

with pytest.raises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

tm.assert_raises_regex would be preferable as you can use it to assert on the message as well. Can still be used as a context manager (can also see usage in test_to_html.py)

Copy link
Contributor Author

@joelostblom joelostblom Jun 1, 2018

Choose a reason for hiding this comment

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

Thanks for the pointers, I updated accordingly.

This test still complains that a ValueError is not raised for any of the parameters. When I call the the function outside the test, it does raise ValueError: ('`text_color_threshold` must be a value from 0 to 1.', 'occurred at index A'). The test works fine if I raise ValueError(msg) manually.

Copy link
Member

Choose a reason for hiding this comment

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

This didn't raise for me locally, though it did when running the _compute method

In [12]: df.style.background_gradient(text_color_threshold='1')
Out[12]: <pandas.io.formats.style.Styler at 0x110163630>

In [13]: df.style.background_gradient(text_color_threshold='1')._compute()
ValueError: ('`text_color_threshold` must be a value from 0 to 1.', 'occurred at index A')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I missed that _compute() is called automatically in Notebooks via _repr_html_. I believe all comments are addressed now.

df.style.background_gradient(
text_color_threshold=text_color_threshold)


def test_block_names():
Expand Down