-
Notifications
You must be signed in to change notification settings - Fork 211
feat: let nw.Enum accept categories, map pandas ordered categorical to Enum (only in main namespace, not stable.v1)
#2192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
3581985
69e9d88
9e63e68
3465fbf
a570751
6f22771
04b2ee6
1393ac7
6e85c1a
70d5c67
e4f2d87
86eb3a2
3e1db9e
5a996b1
1ce56ca
a8f6e42
342ea83
5dd7c72
1bf98d0
da1a455
a79554a
0712557
4fed6d1
f06df25
a0a8c39
21d03dd
36fd178
105e394
0d5b5cc
02b60e0
07d4a25
5e53281
0018a5b
99ad2b5
000013c
6aed0db
3357c55
3b40237
15cac8e
84ea789
833bc3c
7aca1b8
eb0b328
d2504a4
def83a3
f89e331
3902a29
1e3326f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -314,7 +314,9 @@ def rename( | |||
|
|
||||
|
|
||||
| @functools.lru_cache(maxsize=16) | ||||
| def non_object_native_to_narwhals_dtype(dtype: str, version: Version) -> DType: | ||||
| def non_object_native_to_narwhals_dtype(native_dtype: Any, version: Version) -> DType: | ||||
| dtype = str(native_dtype) | ||||
|
|
||||
| dtypes = import_dtypes_module(version) | ||||
| if dtype in {"int64", "Int64", "Int64[pyarrow]", "int64[pyarrow]"}: | ||||
| return dtypes.Int64() | ||||
|
|
@@ -352,7 +354,11 @@ def non_object_native_to_narwhals_dtype(dtype: str, version: Version) -> DType: | |||
| return dtypes.String() | ||||
| if dtype in {"bool", "boolean", "boolean[pyarrow]", "bool[pyarrow]"}: | ||||
| return dtypes.Boolean() | ||||
| if dtype == "category" or dtype.startswith("dictionary<"): | ||||
| if dtype.startswith("dictionary<"): | ||||
| return dtypes.Categorical() | ||||
| if dtype == "category": | ||||
| if native_dtype.ordered: | ||||
| return dtypes.Enum(categories=native_dtype.categories) | ||||
| return dtypes.Categorical() | ||||
|
Comment on lines
+359
to
261
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would be a breaking change, so i'm not totally sure about it - could we preserve the current behaviour in |
||||
| if (match_ := PATTERN_PD_DATETIME.match(dtype)) or ( | ||||
| match_ := PATTERN_PA_DATETIME.match(dtype) | ||||
|
|
@@ -413,7 +419,7 @@ def native_to_narwhals_dtype( | |||
| return arrow_native_to_narwhals_dtype(native_dtype.to_arrow(), version) | ||||
| return arrow_native_to_narwhals_dtype(native_dtype.pyarrow_dtype, version) | ||||
| if str_dtype != "object": | ||||
| return non_object_native_to_narwhals_dtype(str_dtype, version) | ||||
| return non_object_native_to_narwhals_dtype(native_dtype, version) | ||||
| elif implementation is Implementation.DASK: | ||||
| # Per conversations with their maintainers, they don't support arbitrary | ||||
| # objects, so we can just return String. | ||||
|
|
@@ -569,8 +575,17 @@ def narwhals_to_native_dtype( # noqa: PLR0915 | |||
| msg = "PyArrow>=11.0.0 is required for `Date` dtype." | ||||
| return "date32[pyarrow]" | ||||
| if isinstance_or_issubclass(dtype, dtypes.Enum): | ||||
| msg = "Converting to Enum is not (yet) supported" | ||||
| raise NotImplementedError(msg) | ||||
| if isinstance(dtype, dtypes.Enum): | ||||
| try: | ||||
| import pandas as pd # ignore-banned-import | ||||
| except ImportError as exc: # pragma: no cover | ||||
| msg = f"Unable to convert to {dtype} to to the following exception: {exc.msg}" | ||||
| raise ImportError(msg) from exc | ||||
| return pd.CategoricalDtype(categories=dtype.categories, ordered=True) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if we can do something pandas-specific here, as this is used by cudf and modin too - could we generalise?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little confused by this. narwhals/narwhals/_pandas_like/utils.py Line 13 in 5550ad8
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dangotbanned you're right- I pulled this code from a pretty old branch I had so that must have just been leftover. I'll delete it. @MarcoGorelli I'll look into generalizing |
||||
| else: | ||||
| msg = "Can not cast / initialize Enum without categories present" | ||||
| raise ValueError(msg) | ||||
|
|
||||
| if isinstance_or_issubclass( | ||||
| dtype, (dtypes.Struct, dtypes.Array, dtypes.List, dtypes.Time, dtypes.Binary) | ||||
| ): | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import enum | ||
| from collections import OrderedDict | ||
| from datetime import timezone | ||
| from itertools import starmap | ||
|
|
@@ -9,6 +10,8 @@ | |
| from narwhals.utils import isinstance_or_issubclass | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Hashable | ||
| from typing import Iterable | ||
| from typing import Iterator | ||
| from typing import Sequence | ||
|
|
||
|
|
@@ -443,9 +446,32 @@ class Enum(DType): | |
| >>> data = ["beluga", "narwhal", "orca"] | ||
| >>> s_native = pl.Series(data, dtype=pl.Enum(data)) | ||
| >>> nw.from_native(s_native, series_only=True).dtype | ||
| Enum | ||
| Enum(categories=['beluga', 'narwhal', 'orca']) | ||
| """ | ||
|
dangotbanned marked this conversation as resolved.
Outdated
|
||
|
|
||
| def __init__(self, categories: Iterable[Hashable] | type[enum.Enum]) -> None: | ||
| # TODO(Unassigned): pandas errors on NaN, NA, NaT OR duplicated value category | ||
| # Polars errors on Null, NaN OR duplicated OR any non-string category | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MarcoGorelli @dangotbanned any thoughts on whether we should perform a NaN value check at instantiation? Or should we let each backend handle their cases as to these kinds of checks. Note that @dangotbanned and I are contemplating the duplicated/non-string categories in another thread.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd suggest leaving all of these to the backends |
||
| # should the intersection of the above be caught at the narwhals layer? | ||
| if isinstance(categories, type) and issubclass(categories, enum.Enum): | ||
| categories = (getattr(v, "value", v) for v in categories.__members__.values()) | ||
| self.categories = [*categories] | ||
|
|
||
| def __eq__(self: Self, other: object) -> bool: | ||
| # allow comparing object instances to class | ||
| if type(other) is type and issubclass(other, self.__class__): | ||
| return True | ||
| elif isinstance(other, self.__class__): | ||
| return self.categories == other.categories | ||
| return False | ||
|
|
||
| def __hash__(self: Self) -> int: # pragma: no cover | ||
| return hash((self.__class__, tuple(self.categories))) | ||
|
|
||
| def __repr__(self: Self) -> str: # pragma: no cover | ||
| class_name = self.__class__.__name__ | ||
| return f"{class_name}(categories={self.categories!r})" | ||
|
|
||
|
|
||
| class Field: | ||
| """Definition of a single field within a `Struct` DataType. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems to have been there since the first commit (3581985), but doesn't seem to be documented?
It looks like this part is related
https://github.com/camriddell/narwhals/blob/d2504a40efc606d8e626a5b9049ff8054417d64c/narwhals/_pandas_like/utils.py#L320-L321
Which would mean we do the
str(...)call twice now. Just an observation, not sure if there is a cost to thathttps://github.com/camriddell/narwhals/blob/d2504a40efc606d8e626a5b9049ff8054417d64c/narwhals/_pandas_like/utils.py#L306-L309
Are all non-object
pandasdata types guaranteed to be immutable?I think
strwas used because it is hashable, so is safe to use infunctools.lru_cacheThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the cost of a repeated call to
strshould be fairly negligible, we can always come back later to refactor if a profiler disagrees with this statement and this leads to a larger overhead.Since the tests pass, I am at least confident that all of the datatypes are hashable, however whether that hash is something meaningful or just the default
id(self) / 16then caching may not be reliable. That said, perhaps we can also leave as is for now, then if we catch wind of a slow down in the future we can revisit it? Trying to avoid the pre-mature optimization scenarios here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camriddell agreed on the
strpart.My concern on the hashability though is related to #2051 (comment)
Right now we won't get a warning like that because we have:
However - good news!
I changed it to this locally:
And followed though to the docs to find:
Looks like we're all good π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great find on that one, thanks so much for diving in there!