BUG: Incomplete Styler copy methods fix (#39708) #39975

rlukevie · 2021-02-22T20:34:30Z

closes BUG: Styler copy methods not updated for recent additions #39708
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

I haven't made so many pull requests before, so I hope this is going into the right direction.

attack68

looks like a pretty good start. some comments inline.

attack68 · 2021-02-22T21:57:24Z

pandas/io/formats/style.py

            na_rep=self.na_rep,
+            uuid_len=self.uuid_len,


ignore the uuid_len since the uuid is explicitly provided

attack68 · 2021-02-22T22:06:03Z

pandas/io/formats/style.py

@@ -783,15 +783,29 @@ def _copy(self, deepcopy: bool = False) -> Styler:
            precision=self.precision,
            caption=self.caption,
            uuid=self.uuid,


the uuid is suffixed with "_" so need to trim this off here self.uuid[:-1]
edit: actually it might be cleaner and always work if the uuid is manually overwritten after class instantiation.

attack68 · 2021-02-22T22:21:43Z

doc/source/whatsnew/v1.3.0.rst

@@ -482,6 +482,7 @@ Other
 - Bug in :class:`Styler` where rendered HTML was missing a column class identifier for certain header cells (:issue:`39716`)
 - Bug in :meth:`Styler.background_gradient` where text-color was not determined correctly (:issue:`39888`)
 - Bug in :meth:`DataFrame.equals`, :meth:`Series.equals`, :meth:`Index.equals` with object-dtype containing ``np.datetime64("NaT")`` or ``np.timedelta64("NaT")`` (:issue:`39650`)
+- Bug in :class:`Styler` copy methods which were not updated for recently introduced attributes (:issue:`39708`)


can move this up to where other Styler Items are listed

this may not be necessary if this changes were in 1.3 (which they might be); you can add this issue number with another change for Styler (e.g. where the attributes were changed)

this has to be 1.3, since tooltips is the most recent and thats in 1.3.0, referenced in this PR

pandas/io/formats/style.py

attack68 · 2021-02-22T22:30:41Z

pandas/io/formats/style.py

        if deepcopy:
            styler.ctx = copy.deepcopy(self.ctx)
            styler._todo = copy.deepcopy(self._todo)
+            styler.table_styles = copy.deepcopy(self.table_styles)
+            styler.hidden_columns = copy.deepcopy(self.hidden_columns)


think hidden_columns is a list, does it need deepcopy?

attack68 · 2021-02-22T22:35:46Z

pandas/tests/io/formats/test_style.py

+            )
+            self.styler.set_tooltips(ttips)
+            self.styler.cell_ids = not self.styler.cell_ids
+            self.styler.format("{:.2%}")


not making any changes to _todo[]. maybe add .apply(..)

attack68 · 2021-02-22T22:40:19Z

pandas/tests/io/formats/test_style.py

+            self.styler.table_styles[0]["selector"] = "ti"
+            assert self.styler.table_styles != s2.table_styles
+        if do_changes:  # self.styler.tooltips is not None
+            tm.assert_frame_equal(self.styler.tooltips.tt_data, s2.tooltips.tt_data)


possible to wrap the two test cases copy and deepcopy so that dont repeat so much code?

attack68 · 2021-02-22T22:49:40Z

pandas/tests/io/formats/test_style.py

-
+        assert self.styler.table_styles is s2.table_styles
+        assert self.styler.hidden_columns is s2.hidden_columns
+        assert self.styler.cell_context is s2.cell_context


can you group the shallow variables in test to make it clearer?

attack68 · 2021-02-22T22:50:29Z

pandas/tests/io/formats/test_style.py

+        assert self.styler.table_styles == s2.table_styles
+        assert self.styler.table_attributes == s2.table_attributes
+        assert self.styler.cell_ids == s2.cell_ids
+        assert self.styler.uuid_len == s2.uuid_len


uuid is the key variable, uuid_len is only used in construction of uuid test for that instead.

jreback · 2021-02-22T23:17:50Z

doc/source/whatsnew/v1.3.0.rst

@@ -482,6 +482,7 @@ Other
 - Bug in :class:`Styler` where rendered HTML was missing a column class identifier for certain header cells (:issue:`39716`)
 - Bug in :meth:`Styler.background_gradient` where text-color was not determined correctly (:issue:`39888`)
 - Bug in :meth:`DataFrame.equals`, :meth:`Series.equals`, :meth:`Index.equals` with object-dtype containing ``np.datetime64("NaT")`` or ``np.timedelta64("NaT")`` (:issue:`39650`)
+- Bug in :class:`Styler` copy methods which were not updated for recently introduced attributes (:issue:`39708`)


this may not be necessary if this changes were in 1.3 (which they might be); you can add this issue number with another change for Styler (e.g. where the attributes were changed)

rlukevie · 2021-02-23T19:56:52Z

When committing my changes based on your comments, do I always have to add 'BUG: ... (#39708)' to my commit messages? Or is it okay if I just write something like 'Mention issue number in enhancements section'?

I prefer atomic commits, but do all my commits end up upstream after the PR is merged? Shall I do only one single commit for all my changes instead?

attack68 · 2021-02-23T21:19:29Z

When committing my changes based on your comments, do I always have to add 'BUG: ... (#39708)' to my commit messages? Or is it okay if I just write something like 'Mention issue number in enhancements section'?

I prefer atomic commits, but do all my commits end up upstream after the PR is merged? Shall I do only one single commit for all my changes instead?

when merged to master will be squashed to one commit. do as many commits as you want, when you need a new review generally ping for one, wait patiently, or request here. :)

rlukevie · 2021-02-23T23:19:12Z

when merged to master will be squashed to one commit. do as many commits as you want, when you need a new review generally ping for one, wait patiently, or request here. :)

Thanks, and good to know!

rlukevie · 2021-02-26T13:50:42Z

Hi, here's my next try!

jreback · 2021-02-27T19:07:20Z

pandas/io/formats/style.py

@@ -781,16 +781,29 @@ def _copy(self, deepcopy: bool = False) -> Styler:
            self.data,
            precision=self.precision,


is there a reason we actually care about deepcopy, IOW why are we not just always a shallow copy? is this actually used somewhere?

Personally, I have never had use to copy a Styler. But I can see a use case where, in a notebook or report, you might have a dataframe that has some preliminary, shared styling and then some divergence where you highlight different properties as part of a narrative.

I think, off the top of my head, in this scenario you would need a deepcopy to prevent any updates from intermingling with each other, since much of the core styling I think comes from the attributes where the shallow copy will have only single shared pointed reference.

attack68

lgtm.

leaving the discussion about whether copy and/or deepcopy are needed, this I think fixes the existing functionality which has been broken by most of the recent changes.

jreback

lgtm. @rlukevie can you merge master and ping on green

jreback · 2021-03-05T00:52:38Z

thanks @rlukevie

BUG: Incomplete Styler copy methods fix (pandas-dev#39708)

af84c69

attack68 requested changes Feb 22, 2021

View reviewed changes

jreback requested changes Feb 22, 2021

View reviewed changes

attack68 added this to the 1.3 milestone Feb 23, 2021

attack68 added Styler conditional formatting using DataFrame.style Copy / view semantics labels Feb 23, 2021

rlukevie added 6 commits February 23, 2021 22:23

Mention issue number in enhancements section

6e4927f

Improve copy mechanism for uuid and uuid_len

d916e66

Wrap test_copy and test_deepcopy

beae319

Copy hidden_columns, not deepcopy

88ebd70

Remove .format call

be9676a

Simplify test_copy_wrapped

68a19ec

rlukevie added 2 commits February 25, 2021 21:25

Remove duplicate code in test_copy

dc338d5

Merge remote-tracking branch 'upstream/master' into copy-styler

b3dc84d

jreback requested changes Feb 27, 2021

View reviewed changes

attack68 approved these changes Mar 1, 2021

View reviewed changes

jreback requested changes Mar 4, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into copy-styler

c300622

jreback approved these changes Mar 5, 2021

View reviewed changes

jreback merged commit 5dcaf49 into pandas-dev:master Mar 5, 2021

rlukevie deleted the copy-styler branch March 5, 2021 08:42

attack68 mentioned this pull request Mar 29, 2021

API: Styler copy, export, use, clear methods purpose and implementation #40675

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Incomplete Styler copy methods fix (#39708) #39975

BUG: Incomplete Styler copy methods fix (#39708) #39975

rlukevie commented Feb 22, 2021

attack68 left a comment

attack68 Feb 22, 2021

attack68 Feb 22, 2021 •

edited

Loading

attack68 Feb 22, 2021

jreback Feb 22, 2021

attack68 Feb 23, 2021

attack68 Feb 22, 2021

attack68 Feb 22, 2021

attack68 Feb 22, 2021

attack68 Feb 22, 2021

attack68 Feb 22, 2021

jreback Feb 22, 2021

rlukevie commented Feb 23, 2021

attack68 commented Feb 23, 2021

rlukevie commented Feb 23, 2021

rlukevie commented Feb 26, 2021

jreback Feb 27, 2021

attack68 Feb 27, 2021

attack68 left a comment

jreback left a comment

jreback commented Mar 5, 2021

		@@ -781,16 +781,29 @@ def _copy(self, deepcopy: bool = False) -> Styler:
		self.data,
		precision=self.precision,

BUG: Incomplete Styler copy methods fix (#39708) #39975

BUG: Incomplete Styler copy methods fix (#39708) #39975

Conversation

rlukevie commented Feb 22, 2021

attack68 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

attack68 Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlukevie commented Feb 23, 2021

attack68 commented Feb 23, 2021

rlukevie commented Feb 23, 2021

rlukevie commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

attack68 left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback commented Mar 5, 2021

attack68 Feb 22, 2021 •

edited

Loading