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.

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

BUG: fix styler cell_ids arg so that blank style is ignored on False #35588

Merged
merged 5 commits into from
Aug 7, 2020

Conversation

attack68
Copy link
Contributor

@attack68 attack68 commented Aug 6, 2020

@WillAyd
Copy link
Member

WillAyd commented Aug 6, 2020

Can you add a test case for the problem you are trying to solve? As reviewers this is typically the first thing we look for

@WillAyd WillAyd added the Styler conditional formatting using DataFrame.style label Aug 6, 2020
@attack68
Copy link
Contributor Author

attack68 commented Aug 6, 2020

Can you add a test case for the problem you are trying to solve? As reviewers this is typically the first thing we look for

Do you mean in the issue or a test in code?

import pandas as pd  # NOTE VERSION 1.1.0
from pandas.io.formats.style import Styler
def highlight_max(s):
    x = s == s.max()
    x = x.replace(False, '')
    x = x.replace(True, 'background-color: yellow; color: brown')
    return x
df = pd.DataFrame(data=[[0,1], [1,0]])
s = Styler(df, uuid='_', cell_ids=False)
s.apply(highlight_max)
s.render()

yields...

('<style  type="text/css" >\n'
 '#T__row0_col1,#T__row1_col0{\n'
 '            background-color:  yellow;\n'
 '             color:  brown;\n'
 '        }</style><table id="T__" ><thead>    <tr>        <th class="blank '
 'level0" ></th>        <th class="col_heading level0 col0" >0</th>        <th '
 'class="col_heading level0 col1" >1</th>    </tr></thead><tbody>\n'
 '                <tr>\n'
 '                        <th id="T__level0_row0" class="row_heading level0 '
 'row0" >0</th>\n'
 '                        <td id="T__row0_col0" class="data row0 col0" '
 '>0</td>\n'
 '                        <td id="T__row0_col1" class="data row0 col1" '
 '>1</td>\n'
 '            </tr>\n'
 '            <tr>\n'
 '                        <th id="T__level0_row1" class="row_heading level0 '
 'row1" >1</th>\n'
 '                        <td id="T__row1_col0" class="data row1 col0" '
 '>1</td>\n'
 '                        <td id="T__row1_col1" class="data row1 col1" '
 '>0</td>\n'
 '            </tr>\n'
 '    </tbody></table>')

This is clearly erroneous since it has added the id tag to every data cell when (0,0) and (1,1) should be ignored.

Note that the correct output was produced using the same code on pandas version 1.0.4.

The bug fix in this PR yields the code:

('<style  type="text/css" >\n'
 '#T__row0_col1,#T__row1_col0{\n'
 '            background-color:  yellow;\n'
 '             color:  brown;\n'
 '        }</style><table id="T__" ><thead>    <tr>        <th class="blank '
 'level0" ></th>        <th class="col_heading level0 col0" >0</th>        <th '
 'class="col_heading level0 col1" >1</th>    </tr></thead><tbody>\n'
 '                <tr>\n'
 '                        <th id="T__level0_row0" class="row_heading level0 '
 'row0" >0</th>\n'
 '                        <td  class="data row0 col0" >0</td>\n'
 '                        <td id="T__row0_col1" class="data row0 col1" '
 '>1</td>\n'
 '            </tr>\n'
 '            <tr>\n'
 '                        <th id="T__level0_row1" class="row_heading level0 '
 'row1" >1</th>\n'
 '                        <td id="T__row1_col0" class="data row1 col0" '
 '>1</td>\n'
 '                        <td  class="data row1 col1" >0</td>\n'
 '            </tr>\n'
 '    </tbody></table>')

which restores correct functionality since the id tags are removed where appropriate.

@MarcoGorelli
Copy link
Member

@attack68 in the code - see the contributing guide https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#test-driven-development-code-writing

pandas is serious about testing and strongly encourages contributors to embrace test-driven development (TDD). This development process “relies on the repetition of a very short development cycle: first the developer writes an (initially failing) automated test case that defines a desired improvement or new function, then produces the minimum amount of code to pass that test.” So, before actually writing any code, you should write your tests. Often the test can be taken from the original GitHub issue. However, it is always worth considering additional use cases and writing corresponding tests.

Adding tests is one of the most common requests after code is pushed to pandas. Therefore, it is worth getting in the habit of writing tests ahead of time so this is never an issue.

Like many packages, pandas uses pytest and the convenient extensions in numpy.testing.

@attack68
Copy link
Contributor Author

attack68 commented Aug 6, 2020

I wrote what I thought was a very simple test, demonstrating the issue and impact, in the necessary format.
However Linting failed.
Any advice on the reason for the test failure?

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.

can you add a whatsnew note (1.1.1) is ok, put in bug fix section (not regressions)

pandas/tests/io/formats/test_style.py Show resolved Hide resolved
@attack68
Copy link
Contributor Author

attack68 commented Aug 7, 2020

New test failures appeared after the merge by simon. These seem unrelated to my code changes. Any advice?

@jreback jreback added this to the 1.1.1 milestone Aug 7, 2020
@jreback jreback merged commit 16bd49d into pandas-dev:master Aug 7, 2020
@jreback
Copy link
Contributor

jreback commented Aug 7, 2020

thanks @attack68

@simonjayhawkins
Copy link
Member

@meeseeksdev backport to 1.1.x

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Aug 8, 2020
simonjayhawkins pushed a commit that referenced this pull request Aug 8, 2020
…is ignored on False (#35619)

Co-authored-by: attack68 <24256554+attack68@users.noreply.github.com>
@attack68 attack68 deleted the styler_arg_bug branch August 8, 2020 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Styler conditional formatting using DataFrame.style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Pandas Styler cell_ids Arg
5 participants