Style concats #49212

tsvikas · 2022-10-20T16:21:34Z

closes BUG: chaining style.concat overwrites each other #49207
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the doc/source/whatsnew/v1.5.2.rst file if fixing a bug or adding a new feature.

attack68 · 2022-10-21T08:34:01Z

On question before reviewing this properly.

How does this impact the CSS class names of the concatenated stylers?
From memory, you have "data" and "foot_data" if you use
styler.concat(other_styler)
If you do:
styler.concat(other_styler.concat(third_styler))
you have "data" and "foot_data" and "foot_foot_data".

Using your PR i think there is a difference between

styler.concat(other_styler.concat(third_styler)) and styler.concat(other_styler).concat(third_styler) in terms of these.
These must be documented as well as the underlying functionalioty

tsvikas · 2022-10-21T10:39:01Z

It's true. In the original implementation, you'll have _foot got the first concat, _foot_foot for the second, etc.
In this implementation, you'll have just one _foot for all the styles concatenated (at the top level) via chaining.

Should we make them all different? I can try

Also, I haven't seen the original behavior documented anywhere. Have I missed something, or should we document the original behavior also? If so, where?

attack68 · 2022-10-21T11:09:46Z

No I dont think we should make them different. I actually like the way you have implemented this so that you can chain, and any concatenated other will have the same "foot_" prefix.

The ability to create "foot_foot_" is still available due the flexible way this has been implemented.

No this was not documented previously as your issue has raised. With the "notes" section of the styler.concat docs there should be a section on concatenating multiple stylers where these issues are explained.

pandas/io/formats/style_render.py

tsvikas · 2022-11-02T15:40:14Z

Sorry for the delay, I'm definitely want to push this, but didn't had time. Will try to look at it today/tomorrow

…

On Wed, 2 Nov 2022, 17:01 JHM Darbyshire, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/io/formats/style_render.py <#49212 (comment)>: > - for (r, c), v in self.concatenated.ctx.items(): + for (r, c), v in concatenated.ctx.items(): @tsvikas <https://github.com/tsvikas> what do you think of above? This is a good PR so I want to follow up. — Reply to this email directly, view it on GitHub <#49212 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AESBIZOATDTZXI6DQSFTSVDWGJ66HANCNFSM6AAAAAARKKLHUM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

tsvikas · 2022-11-08T15:54:43Z

Sorry for the delay.
I will separate my response into several sections. Hopefully, it'll be more clear that way.

1. I've improved the tests

Now all 3 tests are concatenating 3 different styles, and verifying that they render as expected. They all pass.

2. GHA reports that "Some checks were not successful"

Ubuntu / actions-310.yaml pyarrow=8 not single_cpu (pull_request) Failing after 91m

I have no idea why. I wouldn't expect the tests to pass on all the other systems and fail on this.
Is this a known issue? Can anyone help me investigate?

3. updating `concatenated.ctx` inside the loop

i dont think this will work correctly. The idea here is that the context object on self (the main styler) is updated for the additional rows provided by the other styler, hence why the row number is added to len(self.styler). For adding a list of concatenated styler you will probably need to keep a counter of the rows for all of them.

I will answer in more detail inside the code conversation, but basically - It seems that you are correct. I've tried to code it and it failed the tests. I'm not sure why.

4. I've updated the docs.

hopefully it's OK.

5. same `foot_` prefix is used by all concatenated styles

No I dont think we should make them different. I actually like the way you have implemented this so that you can chain, and any concatenated other will have the same "foot_" prefix.

The ability to create "foot_foot_" is still available due the flexible way this has been implemented.

No this was not documented previously as your issue has raised. With the "notes" section of the styler.concat docs there should be a section on concatenating multiple stylers where these issues are explained.

I'm not sure what is this prefix used for, but maybe differentiating between concatenated styles is important?
maybe we can use foot0_, foot1_, etc. for the concatenated styles in the chain? I assume it'll be easy to implement.
(and s1.concat(s2.concat(s3)) will have a foot0_foot0_ prefix).

In any case, the docs are updated with the current behavior.

tsvikas · 2022-11-08T16:55:07Z

3. updating concatenated.ctx inside the loop

is fixed now.

pandas/io/formats/style_render.py

attack68

I've spotted a few issues.

is HTML that doesn't render correctly because of non-unique cell ids.
is repeated CSS specification. I'm not sure if this is existing issue or new with your code.
is to do with the ctx_len variable. See above question.
is with copying - although we can defer this to a follow up.

doc/source/whatsnew/v1.5.2.rst

pandas/io/formats/style_render.py

…rectly

Co-authored-by: JHM Darbyshire <24256554+attack68@users.noreply.github.com>

tsvikas · 2022-11-16T00:45:33Z

I fixed everything except the copy issue, and did a rebase (hopefully it's ok).

pandas/io/formats/style.py

to explain the css of chained concats

attack68

lgtm - actually just need to push the whats new to 1.5.3, think we missed the window for 1.5.2.

@pandas-dev/pandas-core if anyone else has time to review this as well before merging is great.

tsvikas · 2022-11-22T20:07:19Z

@attack68 mypy fixed. there is another check (doc build and upload) that failed (some warning about numpy deprecation maybe?), and i don't know how to fix that

pandas/io/formats/style.py

attack68 · 2022-11-28T14:45:56Z

Sorry @tsvikas, the whats new is now out of date and needs a merge, although my changes above fixed the checks.

I have been waiting to see if anyone else wants to review, but the code here is quite restricted just to Styler.concat and doesnt affect anything else and is a good extension and fix so I will merge after it s updated.

mroeschke · 2022-11-28T17:35:46Z

pandas/io/formats/style_render.py

+
+        def concatenated_visible_rows(obj):
+            row_indices: list[int] = []
+            _concatenated_visible_rows(obj, 0, row_indices)


Can this function (and the one above) be refactored to just recursively yield each row, since it's just needed in the zip below? From first glance, it seems odd that this is modifying row_indices inplace and the result n isn't used

This doesn't appear to have been addressed (please don't resolve unresolved conversations)

I agree this could be considered for refactor to be made simpler. Does it have to be a blocker for the PR - it is otherwise a very nice addition that addresses a potentially common use case?

Sure can be a follow up but it would be good to add a TODO comment here with the note to simplify

mroeschke · 2022-11-28T17:39:54Z

pandas/io/formats/style.py

            original Styler
+          - ``css`` will be inherited from the original Styler, and the value of
+            keys ``data``, ``row_heading`` and ``row`` will be prepended with
+            ``foot0_``. If more concats are chained, their styles will be prepended


So if there is only 1 Styler, before foot_ would be prepended and now foot0_will be prepended?

This doesn't appear to have been addressed (please don't resolve unresolved conversations)

When chaining multiple Stylers the CSS needs a unique identifier. Previously it was only possible to compound multiple styler concatenations:

styler1.concat(styler2.concat(styler3))

which resulted in CSS classes None, foot_ and foot_foot_.

When allowing chained stylers you need an integer id, so

styler1.concat(styler2).concat(styler3.concat(styler4)).concat(styler5)

results in None, foot0_, foot1_ and foot1_foot0_, foot2_.

The CSS classes were not documented in 1.5.0 previously and not exposed to the user.
Here they are now amended and documented.

So just to confirm, a result with foot_ wasn't visible to the user previously and wouldn't see that foot0_ would now be returned?

foot_ is returned as part of the HTML string in 1.5.0. All of the automatically generated styling CSS ids reference foot_ so that the rendered HTML table, including styles works correctly.

Now the HTML string returned will contain foot0_ and all the auto generated CSS ids will reference that instead.

Unless the user is specifically adding a CSS rule for foot either with an external stylesheet or using set_table_styles there will be no visible difference in the rendered HTML display, either in a JupyterLab or browser.

i.e.

styler2.applymap(lambda v: "color: red;") styler1.concat(styler2)

will display the same in both versions even though the CSS class names have been changed.

Okay thanks for the explanation!

mroeschke

Looks like a 1.5.3.rst got merged already so should use the existing one instead

tsvikas · 2022-11-28T22:55:41Z

i fixed the merge conlict in the whatsnew

pandas/io/formats/style_render.py

Co-authored-by: JHM Darbyshire <24256554+attack68@users.noreply.github.com>

mroeschke

Can merge on greenish

tsvikas · 2022-12-11T10:52:43Z

i'm not sure why ci/circleci: test-arm is red

attack68 · 2022-12-12T07:22:02Z

the failure is unrelated: test_arrow_array

lumberbot-app · 2022-12-12T07:23:33Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout 1.5.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 7ef6a71c5c41eadf9ebea514f4be502e83c95a6f

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #49212: Style concats'

Push to a named branch:

git push YOURFORK 1.5.x:auto-backport-of-pr-49212-on-1.5.x

Create a PR against branch 1.5.x, I would have named this PR:

"Backport PR #49212 on branch 1.5.x (Style concats)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

attack68 · 2022-12-12T07:34:42Z

@tsvikas would you have time to follow the backport instructions above to push the code to the 1.5.x branch?

tsvikas · 2022-12-12T16:35:22Z

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

done, up to "And apply the correct labels and milestones." and "remove the Still Needs Manual Backport label" (which i don't have permission to).

phofl · 2022-12-12T16:36:38Z

done, thx

Allow chaining of style.concat Co-authored-by: Tsvika S <tsvikas@dell> Co-authored-by: JHM Darbyshire <24256554+attack68@users.noreply.github.com> Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

mroeschke added the Styler conditional formatting using DataFrame.style label Oct 20, 2022

attack68 reviewed Oct 21, 2022

View reviewed changes

pandas/io/formats/style_render.py Show resolved Hide resolved

attack68 reviewed Nov 8, 2022

View reviewed changes

pandas/io/formats/style_render.py Outdated Show resolved Hide resolved

tsvikas requested a review from attack68 November 14, 2022 18:18

attack68 requested changes Nov 14, 2022

View reviewed changes

doc/source/whatsnew/v1.5.2.rst Outdated Show resolved Hide resolved

pandas/io/formats/style_render.py Show resolved Hide resolved

pandas/io/formats/style_render.py Outdated Show resolved Hide resolved

Tsvika S and others added 12 commits November 16, 2022 02:02

allow chaining of style.concat

b3bc28b

updated what's new

4d77906

tests: different styles used, to verify that they are all apllied cor…

aa1484f

…rectly

updated docs

6de1944

fix: adding the concatenated to self.ctx correctly

8d41210

added type hint

dd4d29d

Update doc/source/whatsnew/v1.5.2.rst

0d5a258

Co-authored-by: JHM Darbyshire <24256554+attack68@users.noreply.github.com>

minor: change case in docstring

8cfc69c

removed unneeded uuid_len in tests

28d937e

different css classes for different concatenated stylers

6c85396

fixed bug with duplicated css styles, added a relevant test

5703836

refactor: ctx_len is calculated from the concatenated length

4532396

tsvikas requested a review from attack68 November 16, 2022 00:45

attack68 requested changes Nov 16, 2022

View reviewed changes

pandas/io/formats/style.py Outdated Show resolved Hide resolved

updated documentation

fe4be54

to explain the css of chained concats

tsvikas requested a review from attack68 November 17, 2022 19:10

attack68 approved these changes Nov 17, 2022

View reviewed changes

attack68 added the Needs Review label Nov 20, 2022

attack68 reviewed Nov 22, 2022

View reviewed changes

pandas/io/formats/style.py Outdated Show resolved Hide resolved

Update pandas/io/formats/style.py

2698deb

mroeschke reviewed Nov 28, 2022

View reviewed changes

Merge branch 'main' into style-concats

eff3d92

Merge branch 'main' into style-concats

c1cee9d

tsvikas requested a review from mroeschke December 8, 2022 17:32

attack68 reviewed Dec 8, 2022

View reviewed changes

pandas/io/formats/style_render.py Show resolved Hide resolved

Update pandas/io/formats/style_render.py

9cfb970

Co-authored-by: JHM Darbyshire <24256554+attack68@users.noreply.github.com>

mroeschke added this to the 1.5.3 milestone Dec 8, 2022

mroeschke approved these changes Dec 8, 2022

View reviewed changes

style: remove space in EOL

e416060

attack68 merged commit 7ef6a71 into pandas-dev:main Dec 12, 2022

lumberbot-app bot added the Still Needs Manual Backport label Dec 12, 2022

tsvikas mentioned this pull request Dec 12, 2022

Backport PR #49212 on branch 1.5.x (Style concats) #50203

Merged

5 tasks

phofl removed Still Needs Manual Backport Needs Review labels Dec 12, 2022

phofl pushed a commit that referenced this pull request Dec 12, 2022

Backport PR #49212 on branch 1.5.x (Style concats) (#50203)

b2c4d0f

tsvikas deleted the style-concats branch December 12, 2022 20:15

Uh oh!

Style concats #49212

Style concats #49212

Uh oh!

Conversation

tsvikas commented Oct 20, 2022

Uh oh!

attack68 commented Oct 21, 2022

Uh oh!

tsvikas commented Oct 21, 2022

Uh oh!

attack68 commented Oct 21, 2022

Uh oh!

Uh oh!

tsvikas commented Nov 2, 2022 via email

Uh oh!

tsvikas commented Nov 8, 2022

1. I've improved the tests

2. GHA reports that "Some checks were not successful"

3. updating concatenated.ctx inside the loop

4. I've updated the docs.

5. same foot_ prefix is used by all concatenated styles

Uh oh!

tsvikas commented Nov 8, 2022

3. updating concatenated.ctx inside the loop

Uh oh!

Uh oh!

attack68 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tsvikas commented Nov 16, 2022

Uh oh!

Uh oh!

attack68 left a comment

Choose a reason for hiding this comment

Uh oh!

tsvikas commented Nov 22, 2022

Uh oh!

Uh oh!

attack68 commented Nov 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mroeschke Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mroeschke left a comment

Choose a reason for hiding this comment

Uh oh!

tsvikas commented Nov 28, 2022

Uh oh!

Uh oh!

mroeschke left a comment

Choose a reason for hiding this comment

Uh oh!

tsvikas commented Dec 11, 2022

Uh oh!

attack68 commented Dec 12, 2022

Uh oh!

lumberbot-app bot commented Dec 12, 2022

3. updating `concatenated.ctx` inside the loop

5. same `foot_` prefix is used by all concatenated styles

mroeschke Dec 8, 2022 •

edited

Loading

phofl commented Dec 12, 2022 •

edited

Loading