Skip to content

BUG: re-render CSS with styler.apply and applymap non-cleared _todo #39396

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

Merged
merged 6 commits into from
Jan 26, 2021
Merged

BUG: re-render CSS with styler.apply and applymap non-cleared _todo #39396

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
389e5f5
bug re-render styler _todo
attack68 Jan 25, 2021
04a30e5
text comments
attack68 Jan 25, 2021
48818fb
whats new
attack68 Jan 25, 2021
06bcfe0
test
attack68 Jan 25, 2021
aa8d4b4
spaces
attack68 Jan 26, 2021
26b14e4
Merge branch 'master' into styler_bug
attack68 Jan 26, 2021
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
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.3.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ ExtensionArray
Other
^^^^^
- Bug in :class:`Index` constructor sometimes silently ignorning a specified ``dtype`` (:issue:`38879`)
-
- Bug in :class:`Styler` which caused CSS to duplicate on multiple renders. (:issue:`39395`)
-

.. ---------------------------------------------------------------------------
Expand Down
6 changes: 3 additions & 3 deletions pandas/io/formats/style.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,7 @@ def _compute(self):
r = self
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we not just call .clear() first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the _compute method deals only with the _todo list which is the composition of users apply and applymap methods. It populates the ctx object which is later used by _translate to render the HTML.

the clear method removes the _todo list as well as other objects such as Tooltips, the ctx object and the cell_context object from the Styler. You don't want to remove all these at this stage. I think you just want to empty the _todo list after its been 'done'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what user facing effect does this have? can you add a whatsnew note

Everytime a Styler is displayed in JupyterNotebook cell it calls render() and the css is processed (duplicated in the current version). Since HTML just reads the most recent CSS, duplicate CSS doesn't affect the display to user, but for debugging or network transferring the HTML it is best obviously to only generate the css once.

A common user pattern might be to build up styles in notebooks consecutively, meaning multiple cell renders will affect the html output.

Consider 3 cells, so ultimately 3 renders:

 In [1]: df = pd.DataFrame({"A": [0, 1], "B": np.random.randn(2)})
         s = df.style.set_uuid('A_').applymap(lambda x: 'color: green;')
         s
Out[1]: <styled dataframe in green>

In [2]: s.set_uuid('B_').applymap(lambda x: 'font-weight: bold;')
Out [2]: <styled dataframe in bold green>

In [3]: print(s.render())
Out [3]: <style  type="text/css" >
#T_B_row0_col0,#T_B_row0_col1,#T_B_row1_col0,#T_B_row1_col1{
            color:  green;
            color:  green;
            font-weight:  bold;
            color:  green;
            font-weight:  bold;
        }</style>

This PR reduces the final output, without any other implications, to:

<style  type="text/css" >
#T_B_row0_col0,#T_B_row0_col1,#T_B_row1_col0,#T_B_row1_col1{
            color:  green;
            font-weight:  bold;
        }</style>

Copy link
Contributor

Choose a reason for hiding this comment

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

ok can you add a test that replicates this

for func, args, kwargs in self._todo:
r = func(self)(*args, **kwargs)
self._todo = []
return r

def _apply(
Expand Down Expand Up @@ -1454,9 +1455,8 @@ def set_properties(self, subset=None, **kwargs) -> Styler:
>>> df.style.set_properties(color="white", align="right")
>>> df.style.set_properties(**{'background-color': 'yellow'})
"""
values = ";".join(f"{p}: {v}" for p, v in kwargs.items())
f = lambda x: values
return self.applymap(f, subset=subset)
values = "".join(f"{p}: {v};" for p, v in kwargs.items())
return self.applymap(lambda x: values, subset=subset)

@staticmethod
def _bar(
Expand Down
16 changes: 14 additions & 2 deletions pandas/tests/io/formats/test_style.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,24 @@ def test_deepcopy(self):
assert self.styler._todo != s2._todo

def test_clear(self):
s = self.df.style.highlight_max()._compute()
assert len(s.ctx) > 0
tt = DataFrame({"A": [None, "tt"]})
css = DataFrame({"A": [None, "cls-a"]})
s = self.df.style.highlight_max().set_tooltips(tt).set_td_classes(css)
# _todo, tooltips and cell_context items all affected..
assert len(s._todo) > 0
assert s.tooltips
assert len(s.cell_context) > 0
s = s._compute()
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason we are calling private methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is legacy from my perspective. _compute is wrapped up within the render process. Calling just this, rather then say render is just an optimisation of the test, I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, yeah i fyou can just add some blank lines here so its a bit more obvious what is tested

# ctx and _todo items affected
assert len(s.ctx) > 0
assert len(s._todo) == 0 # _todo is emptied after compute.
s._todo = [1]
s.clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

add a line break / comment here

# ctx, _todo, tooltips and cell_context items all revert to null state.
assert len(s.ctx) == 0
assert len(s._todo) == 0
assert not s.tooltips
assert len(s.cell_context) == 0

def test_render(self):
df = DataFrame({"A": [0, 1]})
Expand Down