feat(python): Deprecate `with_column` #6128

zundertj · 2023-01-08T22:12:47Z

See #6117. As you can see, this is a big change.

TODO's:

There are two Python tests failing, as it seems that the behaviour between with_column and with_columns is different for Series. I need to look into this.
tests for the deprecation warning being raised have to be added
I haven't touched the Rust api yet, would we want to do the same there?

Probably later this week done.

ritchie46 · 2023-01-09T07:15:45Z

I haven't touched the Rust api yet, would we want to do the same there?

In rust it is very common to have similar named methods for different arguments. One takes an expression the other takes a sequence of expressions. I think we should leave it like that.

zundertj · 2023-01-13T20:11:21Z

Ok, the failing tests were passing in a pl.Series into LazyFrame.with_columns, whilst that was fine in DataFrame.with_columns. Reason is that DataFrame.with_columns was wrapping it in a list, and then passed it on to LazyFrame.with_columns. I have moved that logic inside LazyFrame.with_columns, added a unit test to verify and now DataFrame.with_columns is the minimum possible wrapper around LazyFrame.with_columns, so there should be no inconsistency.

As a side note, I see that we handle things differently for args vs named_args in with_columns, where the latter has more flexibility (or unclear api), but lets leave that to a separate PR.

zundertj · 2023-01-13T20:15:10Z

py-polars/polars/internals/lazyframe/frame.py

-        exprs = (
-            []
-            if exprs is None
-            else ([exprs] if isinstance(exprs, pli.Expr) else list(exprs))


Sorry, my brain cant handle this on a Friday night...

zundertj · 2023-01-13T20:16:01Z

py-polars/polars/internals/dataframe/frame.py

@@ -5568,8 +5577,6 @@ def with_columns(
        └─────┴──────┴───────┴──────┴───────┘

        """
-        if exprs is not None and not isinstance(exprs, Sequence):
-            exprs = [exprs]


This is now gone

ritchie46 · 2023-01-14T07:56:59Z

Is everybody good to go on this one? I don't want to merge this immediately, but I want to add the deprecation on the breaking release I plan end of this month. This is core functionality IMO and I think we are becoming used enough that we should introduce a bit of a grace period.

zundertj · 2023-01-14T08:06:08Z

This will cause for many users deprecation warnings, so please double/triple check that we are happy with this indeed.

alexander-beedie · 2023-01-14T08:54:03Z

This will cause for many users deprecation warnings, so please double/triple check that we are happy with this indeed.

Looks like a winner to me...

One thing: I'd make the DeprecationWarning text "This method will be removed in the future" more specific, eg: "This method will be removed shortly" (and maybe even mention "in 0.16")

stinodego · 2023-01-17T13:02:08Z

I think this is a good move!

And we don't have to remove with_column in 0.16.0, right? As it is widely used, I would suggest to just leave it in until 0.17.0.

I agree with @alexander-beedie that we should include this version information in the deprecation message, to help users schedule their upgrade.

Also, I am going to rebrand this as an 'enhancement', since this affects our public API.

py-polars/polars/internals/dataframe/frame.py

py-polars/polars/internals/lazyframe/frame.py

py-polars/polars/internals/dataframe/frame.py

zundertj · 2023-01-17T22:22:48Z

Rebased & squashed.

zundertj · 2023-01-17T22:25:57Z

I think this is a good move!

And we don't have to remove with_column in 0.16.0, right? As it is widely used, I would suggest to just leave it in until 0.17.0.

I agree with @alexander-beedie that we should include this version information in the deprecation message, to help users schedule their upgrade.

Also, I am going to rebrand this as an 'enhancement', since this affects our public API.

I had on purpose not added in a version, because we don't know (yet). I think, given that the warnings will be annoying, people would update relatively quickly their code after the Polars upgrade, but we will have to cater to users who update Polars infrequent. So I did not want to pin us down yet on a timeline. But if there is a good idea for how long it will be until 0.16 or 0.17 land, we can put that in.

stinodego · 2023-01-18T10:45:41Z

Fair enough. Maybe @ritchie46 can comment on this. Once we decide on what the deprecation message should be, this PR should be good to go!

ritchie46 · 2023-01-18T12:40:24Z

Just to be certain, we now accept

a single expression
a list of expressions
multiple expressions as args

in with_columns right?

stinodego · 2023-01-18T13:38:24Z

Yes, that's correct! And also Series / list of Series.

ritchie46 · 2023-01-18T13:47:07Z

Alright. Thanks all! On the message. I think 0.17 or 0.18 seems a reasonable time duration?

stinodego · 2023-01-18T13:55:21Z

I'd say 0.17.0 is fine, as it's still quite far away.

zundertj · 2023-01-19T20:58:38Z

Added 0.17 to docstring and the warning.

As `with_columns` has a superset of functionality. Remove test script Synchronize input arg parsing between DataFrame and LazyFrame with_columns Update py-polars/polars/internals/dataframe/frame.py Co-authored-by: Stijn de Gooijer <stijn@degooijer.io> Update py-polars/polars/internals/lazyframe/frame.py Co-authored-by: Stijn de Gooijer <stijn@degooijer.io> Update py-polars/polars/internals/dataframe/frame.py Co-authored-by: Stijn de Gooijer <stijn@degooijer.io> Update docstrings Fix doctests Revert Cargo.lock Specify v0.17 in deprecation warnings Fix faulty merge

stinodego · 2023-01-25T12:24:00Z

@ritchie46 is there still some doubt or unclarity about this change?

If not, I think we should go ahead and merge this, as it's big and very prone to merge conflicts.

ritchie46 · 2023-01-25T14:36:52Z

Alright! Thanks all!

timreuscher · 2023-01-26T18:04:49Z

Hadn't updated since 0.14.18, just installed 0.15.18, ran some code, saw the warning which made complete sense in both the warning displayed and the change it warned about, did a few search-and-replaces (I guess that's the plural) and no more warnings in less than five minutes.

Just wanted to leave my two cents as an occasional user out in the field that this is an excellent example of how to deprecate old methods. Well done!

stinodego · 2023-01-26T18:32:29Z

Thanks! Always nice to hear some positive feedback 😸

zundertj mentioned this pull request Jan 8, 2023

Rationalize with_column & with_columns #6117

Closed

zundertj marked this pull request as draft January 8, 2023 22:15

zundertj force-pushed the deprecate_with_column branch from afc3d5a to 8525792 Compare January 13, 2023 20:08

zundertj changed the title ~~Deprecate with_column~~ refactor(python): Deprecate with_column Jan 13, 2023

github-actions bot added python Related to Python Polars refactor Code improvements that do not change functionality labels Jan 13, 2023

zundertj commented Jan 13, 2023

View reviewed changes

zundertj marked this pull request as ready for review January 13, 2023 20:26

stinodego reviewed Jan 17, 2023

View reviewed changes

py-polars/polars/internals/dataframe/frame.py Outdated Show resolved Hide resolved

stinodego reviewed Jan 17, 2023

View reviewed changes

py-polars/polars/internals/lazyframe/frame.py Outdated Show resolved Hide resolved

stinodego reviewed Jan 17, 2023

View reviewed changes

py-polars/polars/internals/dataframe/frame.py Outdated Show resolved Hide resolved

stinodego reviewed Jan 17, 2023

View reviewed changes

py-polars/polars/internals/dataframe/frame.py Outdated Show resolved Hide resolved

stinodego changed the title ~~refactor(python): Deprecate with_column~~ feat(python): Deprecate with_column Jan 17, 2023

stinodego removed the refactor Code improvements that do not change functionality label Jan 17, 2023

github-actions bot added the enhancement New feature or an improvement of an existing feature label Jan 17, 2023

zundertj force-pushed the deprecate_with_column branch 3 times, most recently from 9621946 to a05308a Compare January 17, 2023 22:22

zundertj force-pushed the deprecate_with_column branch 2 times, most recently from 30ee907 to 30bddf8 Compare January 19, 2023 20:58

zundertj force-pushed the deprecate_with_column branch from 30bddf8 to ad0e0fe Compare January 20, 2023 06:45

stinodego approved these changes Jan 20, 2023

View reviewed changes

zundertj force-pushed the deprecate_with_column branch from ad0e0fe to e58febb Compare January 22, 2023 14:26

zundertj force-pushed the deprecate_with_column branch from e58febb to 3a69b26 Compare January 23, 2023 21:46

ritchie46 merged commit e4c71b9 into pola-rs:master Jan 25, 2023

This was referenced Apr 28, 2023

Unexpected arguments pola-rs/polars-benchmark#41

Closed

Updating polars kwargs for scan/read_csv and LazyFrame operations pola-rs/polars-benchmark#44

Merged

sorhawell mentioned this pull request Jul 2, 2023

What is the difference between with_column() and with_columns()? pola-rs/r-polars#290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(python): Deprecate `with_column` #6128

feat(python): Deprecate `with_column` #6128

zundertj commented Jan 8, 2023 •

edited

Loading

ritchie46 commented Jan 9, 2023

zundertj commented Jan 13, 2023 •

edited

Loading

zundertj Jan 13, 2023

zundertj Jan 13, 2023

ritchie46 commented Jan 14, 2023

zundertj commented Jan 14, 2023

alexander-beedie commented Jan 14, 2023 •

edited

Loading

stinodego commented Jan 17, 2023 •

edited

Loading

zundertj commented Jan 17, 2023

zundertj commented Jan 17, 2023 •

edited

Loading

stinodego commented Jan 18, 2023

ritchie46 commented Jan 18, 2023

stinodego commented Jan 18, 2023 •

edited

Loading

ritchie46 commented Jan 18, 2023 •

edited

Loading

stinodego commented Jan 18, 2023

zundertj commented Jan 19, 2023

stinodego commented Jan 25, 2023

ritchie46 commented Jan 25, 2023

timreuscher commented Jan 26, 2023 •

edited

Loading

stinodego commented Jan 26, 2023

feat(python): Deprecate with_column #6128

feat(python): Deprecate with_column #6128

Conversation

zundertj commented Jan 8, 2023 • edited Loading

ritchie46 commented Jan 9, 2023

zundertj commented Jan 13, 2023 • edited Loading

zundertj Jan 13, 2023

Choose a reason for hiding this comment

zundertj Jan 13, 2023

Choose a reason for hiding this comment

ritchie46 commented Jan 14, 2023

zundertj commented Jan 14, 2023

alexander-beedie commented Jan 14, 2023 • edited Loading

stinodego commented Jan 17, 2023 • edited Loading

zundertj commented Jan 17, 2023

zundertj commented Jan 17, 2023 • edited Loading

stinodego commented Jan 18, 2023

ritchie46 commented Jan 18, 2023

stinodego commented Jan 18, 2023 • edited Loading

ritchie46 commented Jan 18, 2023 • edited Loading

stinodego commented Jan 18, 2023

zundertj commented Jan 19, 2023

stinodego commented Jan 25, 2023

ritchie46 commented Jan 25, 2023

timreuscher commented Jan 26, 2023 • edited Loading

stinodego commented Jan 26, 2023

feat(python): Deprecate `with_column` #6128

feat(python): Deprecate `with_column` #6128

zundertj commented Jan 8, 2023 •

edited

Loading

zundertj commented Jan 13, 2023 •

edited

Loading

alexander-beedie commented Jan 14, 2023 •

edited

Loading

stinodego commented Jan 17, 2023 •

edited

Loading

zundertj commented Jan 17, 2023 •

edited

Loading

stinodego commented Jan 18, 2023 •

edited

Loading

ritchie46 commented Jan 18, 2023 •

edited

Loading

timreuscher commented Jan 26, 2023 •

edited

Loading