depr(python,rust!): Rename DataFrame.melt to unpivot and make parameters consistent with pivot

[MarcoGorelli]

Closes #11974

The diff isn't super-clear in one bit, but py-polars/tests/unit/operations/test_melt.py is just renamed to py-polars/tests/unit/operations/test_unpivot.py but with an extra couple of with pytest.deprecated_call

[codecov]

Codecov Report

Attention: Patch coverage is 88.17204% with 11 lines in your changes missing coverage. Please review.

Project coverage is 80.87%. Comparing base (8a6bf4b) to head (1ad45cf).

❗ Current head 1ad45cf differs from pull request most recent head a633eca

Please upload reports for the commit a633eca to get more accurate results.

Files	Patch %	Lines
crates/polars-plan/src/plans/functions/mod.rs	44.44%	5 Missing ⚠️
py-polars/src/lazyframe/visitor/nodes.rs	0.00%	4 Missing ⚠️
crates/polars-plan/src/plans/functions/dsl.rs	50.00%	1 Missing ⚠️
...plan/src/plans/optimizer/predicate_pushdown/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17095      +/-   ##
==========================================
+ Coverage   80.86%   80.87%   +0.01%     
==========================================
  Files        1456     1456              
  Lines      191141   191137       -4     
  Branches     2728     2728              
==========================================
+ Hits       154562   154585      +23     
+ Misses      36073    36046      -27     
  Partials      506      506              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stinodego]

Whew, quite a lot of churn to get through it looks like!

I know this is still in draft, but I wanted to mention: we should keep melt but deprecate it, so we can remove it with 2.0.0.

[MarcoGorelli]

ah sorry, when you said to get rid of melt I may have taken you too literally 😄 ok, will update tomorrow to just make it a deprecated alias

[Julian-J-S]

Love it! 😄

Question:
what about the columns parameter of pivot?
Is this also going to be renamed to on like I suggested in my issue?

Asking because the idea was to align the parameters of pivot/unpivot.

[ritchie46]

Alright! Ready to undraft?

[MarcoGorelli]

almost, just finishing up pivot

[MarcoGorelli]

Sure, done

I do like how it becomes symmetrical, which probably makes things easier for new users, so thanks @JulianCologne for the suggestion. There is some code churn for existing users, but it's a fairly easy find-and-replace

[stinodego]

Thanks Marco, looks good!

Just one question about the design: What do you think about making on the first parameter, like suggested in the linked issue? No strong opinion myself, just checking.

[MarcoGorelli]

For pivot I don't think that can be done in a non-breaking way, at least not without first making the arguments keyword-only. And so if the order in pivot stays <index, on>, then it should probably stay that way too in unpivot?

[stinodego]

For pivot I don't think that can be done in a non-breaking way, at least not without first making the arguments keyword-only. And so if the order in pivot stays <index, on>, then it should probably stay that way too in unpivot?

For pivot, non-keyword args were already deprecated, related to this issue: #12087

I had just removed this deprecation for 1.0, so we can change their order without breaking anything right now. So we are really free in determining the most intuitive API here. Would it be better with on first?

[MarcoGorelli]

ah, cool! in that case, agree, on would look better first

[MarcoGorelli]

I had just removed this deprecation for 1.0, so we can change their order without breaking anything right now

not sure I understand sorry - am I OK to make the arguments keyword-only as part of this PR? It still needs enforcing, right?

[stinodego]

not sure I understand sorry - am I OK to make the arguments keyword-only as part of this PR? It still needs enforcing, right?

You don't need to make them keyword-only, you can just move the parameters as you see fit.

If you look at the latest release (0.20.31), you can see that non-keyword arguments were already deprecated:
https://github.com/pola-rs/polars/blob/py-0.20.31/py-polars/polars/dataframe/frame.py#L7561-L7780

So at this point, everyone has their args as keyword arguments (theoretically speaking). So we are free to move around the parameters as we see fit for the 1.0 release.

[MarcoGorelli]

I feel a bit uneasy about this one, I think it's going to silently break people's code. Though I think the current change in argument order is already going to break it anyway, so let's think if there's a better way

I think the following would feel natural, and would loudly break for anyone who didn't pay attention to their warnings (based on some clients I've worked with, I'd estimate that that's the majority 😩 ):

def pivot(self, on, *, values=None, index=None): ...

Then the call becomes:

df.pivot('subject', index='name', values=['test_1', 'test_2'])

if you don't specify either of index or values, then all the remaining columns are used for that argument (I think Marshall was suggesting this some time ago). But you need to specify at least one of index or values

If we could do that, I'd feel a lot easier about changing the order. I also think it would lead to more readable code if index and values are keyword-only, else df.pivot('a', 'b', 'c') could do anything as far as the uninitiated reader is concerned

[stinodego]

Sure that sounds good. Let's do on, *, index, values though, because values is optional. And unpivot also starts with on, *, index then.

[stinodego]

Looks great, thanks Marco!