feat: deprecate Expr.head, Expr.tail, Expr.sort, Expr.gather_every, Expr.sample (but keep them in stable.v1) #1791
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
MarcoGorelli
force-pushed
the
remove-non-agg-shape-changes
branch
2 times, most recently
from
5b92ccb to
20ef637
Compare
MarcoGorelli
commented
Jan 11, 2025
Comment on lines
-11
to
+12
| def test_head( | ||
| def test_tail( |
MarcoGorelli
changed the title
MarcoGorelli
force-pushed
the
remove-non-agg-shape-changes
branch
from
20ef637 to
655616e
Compare
MarcoGorelli
changed the title
MarcoGorelli
changed the title
MarcoGorelli
changed the title
EdAbati
approved these changes
Jan 15, 2025
FBruzzesi
approved these changes
Jan 16, 2025
| @@ -21,6 +22,9 @@ def test_gather_every_expr( | |||
|
|
|||
| assert_equal_data(result, expected) | |||
|
|
|||
| with pytest.deprecated_call(): | |||
FBruzzesi
changed the title
|
thanks all for feedback and for taking a look! i think more than implementation difficulties they're just not relational, e.g. In [16]: df
Out[16]:
shape: (3, 2)
┌───────┬─────┐
│ name ┆ age │
│ --- ┆ --- │
│ str ┆ i64 │
╞═══════╪═════╡
│ Mario ┆ 31 │
│ Luigi ┆ 32 │
│ Wario ┆ 45 │
└───────┴─────┘
In [17]: df.select('name', pl.col('age').sort(descending=True))
Out[17]:
shape: (3, 2)
┌───────┬─────┐
│ name ┆ age │
│ --- ┆ --- │
│ str ┆ i64 │
╞═══════╪═════╡
│ Mario ┆ 45 │
│ Luigi ┆ 32 │
│ Wario ┆ 31 │
└───────┴─────┘you end up with tuples which aren't part of the original object - it's fine for Polars to allow it, but I'm not sure we should be allowing this kind of operation in Narwhals In any case, happy to bring these back if use cases do come along! |
MarcoGorelli
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One step closer to
narwhals.stable.v2These are some operations which we won't be able to support in the lazy layer at all, and I think it would be confusing to have some Expr methods which aren't available at all for lazyframes (which would be different to
Expr.unique, which could be supported if followed by an aggregation, orExpr.cum_sum, which could be supported iforder_byis passed)Further, I just don't think any of these expressions are very useful...people call head/tail/sort/sample on dataframes, not typically on individual expressions..
For the cases where people need them, they can use DataFrame or Series operations
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below