feat: Spec'd-out CompliantExpr
#2119
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| from typing import Any | ||
| from typing import Callable | ||
| from typing import Literal | ||
| from typing import Mapping | ||
| from typing import Sequence | ||
|
|
||
| from narwhals._dask.expr_dt import DaskExprDateTimeNamespace | ||
|
|
@@ -20,6 +21,7 @@ | |
| from narwhals.typing import CompliantExpr | ||
| from narwhals.utils import Implementation | ||
| from narwhals.utils import generate_temporary_column_name | ||
| from narwhals.utils import not_implemented | ||
|
|
||
| if TYPE_CHECKING: | ||
| try: | ||
|
|
@@ -382,7 +384,11 @@ def drop_nulls(self: Self) -> Self: | |
| return self._from_call(lambda _input: _input.dropna(), "drop_nulls") | ||
|
|
||
| def replace_strict( | ||
| self: Self, | ||
| old: Sequence[Any] | Mapping[Any, Any], | ||
| new: Sequence[Any], | ||
| *, | ||
| return_dtype: DType | type[DType] | None, | ||
| ) -> Self: | ||
| msg = "`replace_strict` is not yet supported for Dask expressions" | ||
| raise NotImplementedError(msg) | ||
|
|
@@ -527,7 +533,7 @@ def null_count(self: Self) -> Self: | |
| lambda _input: _input.isna().sum().to_series(), "null_count" | ||
| ) | ||
|
|
||
| def over(self: Self, keys: Sequence[str], kind: ExprKind) -> Self: | ||
| def func(df: DaskLazyFrame) -> list[Any]: | ||
| output_names, aliases = evaluate_output_names_and_aliases(self, df, []) | ||
| if overlap := set(output_names).intersection(keys): | ||
|
|
@@ -540,9 +546,10 @@ def func(df: DaskLazyFrame) -> list[Any]: | |
| raise NotImplementedError(msg) | ||
| if df._native_frame.npartitions == 1: # pragma: no cover | ||
| tmp = df.group_by(*keys, drop_null_keys=False).agg(self) | ||
| on = list(keys) | ||
| tmp_native = ( | ||
| df.simple_select(*keys) | ||
| .join(tmp, how="left", left_on=on, right_on=on, suffix="_right") | ||
| ._native_frame | ||
| ) | ||
| return [tmp_native[name] for name in aliases] | ||
|
|
@@ -586,3 +593,23 @@ def dt(self: Self) -> DaskExprDateTimeNamespace: | |
| @property | ||
| def name(self: Self) -> DaskExprNameNamespace: | ||
| return DaskExprNameNamespace(self) | ||
|
|
||
| arg_min = not_implemented("arg_min") | ||
| arg_max = not_implemented("arg_max") | ||
| arg_true = not_implemented("arg_true") | ||
| head = not_implemented("head") | ||
| tail = not_implemented("tail") | ||
| mode = not_implemented("mode") | ||
| sort = not_implemented("sort") | ||
| rank = not_implemented("rank") | ||
| sample = not_implemented("sample") | ||
|
Member
Author
There was a problem hiding this comment. Note If we went with this - we'd also need to revise https://github.com/narwhals-dev/narwhals/blob/77a0150ffb5121086d74b0f9a10ec26781162529/utils/generate_backend_completeness.py
Member
There was a problem hiding this comment. Is there a particular reason for this implementation over a My thoughts boil down to
alternatively, we could have Here's a couple of examples for the current impl vs decorator vs descriptor. # current
class T:
arg_min = not_implemented("arg_min")
# decorator pattern
class T:
@not_implemented
def arg_min(self):
... # elipsis to indicate that this just hasn't been written yet
# descriptor
class T:
arg_min = not_implemented()
Member
Author
There was a problem hiding this comment. @camriddell glad to see you here! I will try to respond in a bit more detail tomorrow, but on this point specifically:
I've got a descriptor implementation that solves that issue I'm leaning towards that most strongly at the moment - but it is mainly used in tests only at the moment (
Member
Author
There was a problem hiding this comment.
@camriddell I've added this (no talking) video to try and show off what the improved typing lets you do instead of grepping the code base: 2025-03-04.13-13-19.-.Expr-Not.Implemented-30-1.mp4Important We can now statically know what is and is not implemented in VSCode
This gives us a clear line between not/implemented while also being very compact. Adding decorated signatures for just 4 methods creates this diff: diff --git a/narwhals/_dask/expr.py b/narwhals/_dask/expr.py
index a1bfb0b8..41c86132 100644
--- a/narwhals/_dask/expr.py
+++ b/narwhals/_dask/expr.py
@@ -25,6 +25,7 @@ from narwhals.utils import Implementation
from narwhals.utils import generate_temporary_column_name
from narwhals.utils import not_implemented
from narwhals.utils import not_implemented_alt
+from narwhals.utils import unstable # <------------------------ using just for syntactically valid decorator demo
if TYPE_CHECKING:
try:
@@ -615,10 +616,27 @@ class DaskExpr(CompliantExpr["DaskLazyFrame", "dx.Series"]): # pyright: ignore[
sample = not_implemented("sample")
map_batches = not_implemented("map_batches")
ewm_mean = not_implemented("ewm_mean")
- rolling_sum = not_implemented("rolling_sum")
- rolling_mean = not_implemented("rolling_mean")
- rolling_var = not_implemented("rolling_var")
- rolling_std = not_implemented("rolling_std")
+
+ @unstable
+ def rolling_mean(
+ self, window_size: int, *, min_samples: int | None, center: bool
+ ) -> Self: ...
+
+ @unstable
+ def rolling_std(
+ self, window_size: int, *, min_samples: int | None, center: bool, ddof: int
+ ) -> Self: ...
+
+ @unstable
+ def rolling_sum(
+ self, window_size: int, *, min_samples: int | None, center: bool
+ ) -> Self: ...
+
+ @unstable
+ def rolling_var(
+ self, window_size: int, *, min_samples: int | None, center: bool, ddof: int
+ ) -> Self: ...
+
gather_every = not_implemented("gather_every")
replace_strict = not_implemented_alt()
They now all fail type checking - because >>> mypy
narwhals/_dask/expr.py:621: error: Missing return statement [empty-body]
def rolling_mean(
^
narwhals/_dask/expr.py:621: note: If the method is meant to be abstract, use @abc.abstractmethod
narwhals/_dask/expr.py:626: error: Missing return statement [empty-body]
def rolling_std(
^
narwhals/_dask/expr.py:626: note: If the method is meant to be abstract, use @abc.abstractmethod
narwhals/_dask/expr.py:631: error: Missing return statement [empty-body]
def rolling_sum(
^
narwhals/_dask/expr.py:631: note: If the method is meant to be abstract, use @abc.abstractmethod
narwhals/_dask/expr.py:636: error: Missing return statement [empty-body]
def rolling_var(
^
narwhals/_dask/expr.py:636: note: If the method is meant to be abstract, use @abc.abstractmethod
Found 4 errors in 1 file (checked 348 source files)But lets say that wasn't an issue.
Member
Author
There was a problem hiding this comment. @camriddell In my big PR description ramble I mentioned that this was where the PR started.
This comment was marked as outdated.
Sorry, something went wrong. |
||
| map_batches = not_implemented("map_batches") | ||
| ewm_mean = not_implemented("ewm_mean") | ||
| rolling_sum = not_implemented("rolling_sum") | ||
| rolling_mean = not_implemented("rolling_mean") | ||
| rolling_var = not_implemented("rolling_var") | ||
| rolling_std = not_implemented("rolling_std") | ||
| gather_every = not_implemented("gather_every") | ||
|
|
||
| cat = not_implemented("cat", is_property=True) | ||
| list = not_implemented("list", is_property=True) | ||
Uh oh!
There was an error while loading. Please reload this page.