Skip to content

BUG: styler multiindex hiding indexes and columns alignment #43649

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 58 commits into from
Oct 18, 2021

Conversation

attack68
Copy link
Contributor

@attack68 attack68 commented Sep 18, 2021

Follows #43686

This PR has two parts:

  1. Addressing the bug fixes as per above.

The key change made here was to add a single line: if r not in self.hidden_rows: in the loop, and in _translate_latex update for the new object structure that is input to it.

  1. Refactoring the code so that the main part of the code is now easier to grok:
def _translate(...):
    # calls _translate_header()
    # callls _translate_body()

def _translate_header(...):
   # calls _generate_col_header_row()
   # calls _generate_index_names_row()

def _translate_body(...):
   # calls _generate_trimmed_row()
   # calls _generate_body_row()

And tests were update and comprehensive tests for the issues added.

)
for c in range(max_cols)
]
if r not in self.hidden_rows:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only added this line to this method and all code below it was indended one tab.

Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth splitting up translate_body to some function calls to make it simpler to grok (future)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeh agreed, i already did this once splitting _translate into header and body and now it has acquired some more stuff, like min/mix limiting etc.. will add an issue

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 will finish making this clean up after #43686

@attack68 attack68 changed the title Bug: styler multiindex hiding indexes and columns alignment BUG: styler multiindex hiding indexes and columns alignment Sep 19, 2021
@attack68 attack68 added Bug IO HTML read_html, to_html, Styler.apply, Styler.applymap Styler conditional formatting using DataFrame.style IO LaTeX to_latex labels Sep 20, 2021
for r, row in enumerate(d["body"]):
index_levels = self.data.index.nlevels
for r, row in zip(
[r for r in range(len(self.data.index)) if r not in self.hidden_rows],
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth a property to have this

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 only needed for latex, so for most cases not necessary and might add additional overhead for large HTML (usually latex tables quite a lot smaller)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

took a second look here. d is dynamic so a class property won't work. think the comment is sufficient.

…ndex_hiding

# Conflicts:
#	pandas/tests/io/formats/style/test_to_latex.py
@attack68
Copy link
Contributor Author

attack68 commented Sep 22, 2021

because of moving the hiding functions to outer loop:

before           after         ratio
     [cd13e3a4]       [75de033c]
     <master>         <bug_styler_multiindex_hiding>
-      11.2±0.1ms       9.44±0.3ms     0.85  io.style.Render.time_apply_format_hide_render(24, 12)
-     6.57±0.09ms      5.39±0.06ms     0.82  io.style.Render.time_apply_format_hide_render(12, 12)
-        19.2±7ms       12.2±0.2ms     0.64  io.style.Render.time_apply_format_hide_render(36, 12)
-      45.1±0.6ms       20.7±0.5ms     0.46  io.style.Render.time_apply_format_hide_render(24, 120)
-        26.6±3ms       11.9±0.5ms     0.45  io.style.Render.time_apply_format_hide_render(12, 120)
-      68.6±0.8ms       28.8±0.2ms     0.42  io.style.Render.time_apply_format_hide_render(36, 120)

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

…classes

# Conflicts:
#	doc/source/user_guide/style.ipynb
…ndex_hiding

# Conflicts:
#	doc/source/user_guide/style.ipynb
#	doc/source/whatsnew/v1.4.0.rst
#	pandas/io/formats/style.py
#	pandas/io/formats/style_render.py
#	pandas/tests/io/formats/style/test_html.py
…ndex_hiding

# Conflicts:
#	doc/source/user_guide/style.ipynb
#	doc/source/whatsnew/v1.4.0.rst
#	pandas/io/formats/style.py
#	pandas/io/formats/style_render.py
#	pandas/tests/io/formats/style/test_html.py
@attack68 attack68 requested a review from jreback October 18, 2021 19:50
@jreback jreback merged commit c5ccc18 into pandas-dev:master Oct 18, 2021
@jreback
Copy link
Contributor

jreback commented Oct 18, 2021

thanks!

@attack68 attack68 deleted the bug_styler_multiindex_hiding branch October 19, 2021 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug IO HTML read_html, to_html, Styler.apply, Styler.applymap IO LaTeX to_latex Styler conditional formatting using DataFrame.style
Projects
None yet
2 participants