-
Notifications
You must be signed in to change notification settings - Fork 168
chore: Add DTypeClass to improve DType.__repr__ and simplify __slots__
#3213
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
base: main
Are you sure you want to change the base?
Conversation
| @pytest.mark.parametrize("dtype_name", ["Datetime", "Duration", "Enum"]) | ||
| def test_dtype_repr_versioned(dtype_name: str) -> None: | ||
| from narwhals.stable import v1 as nw_v1 | ||
|
|
||
| dtype_class_main = getattr(nw, dtype_name) | ||
| dtype_class_v1 = getattr(nw_v1, dtype_name) | ||
|
|
||
| assert dtype_class_main is not dtype_class_v1 | ||
| assert repr(dtype_class_main) != repr(dtype_class_v1) |
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.
I pushed this (failing) test as I've been thinking about if we want to distinguish 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.
I think it could make sense to distinguish between the two...buuuut do we have a valid proposal? There is none I can think of that I really like to be honest with you π€
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.
Detour
I experimented with this a little bit in #2571
narwhals/narwhals/_plan/expr.py
Lines 56 to 61 in 1ab1599
| class Expr: | |
| _ir: ir.ExprIR | |
| _version: ClassVar[Version] = Version.MAIN | |
| def __repr__(self) -> str: | |
| return f"nw._plan.Expr({self.version.name.lower()}):\n{self._ir!r}" |
Which looks like:
import narwhals._plan as nwp
nwp.col("a").drop_nulls().alias("b")nw._plan.Expr(main):
col('a').drop_nulls().alias('b')But I'd forgotten I did that, since _repr_html_ overrides that:
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.
Proposal
But since we only have 3x DTypes, maybe something like:
import narwhals as nw
from narwhals.dtypes import DType
from narwhals._utils import Version
def repr_dtype_class(dtype: type[DType], version: Version) -> str:
v = f"[{version.name.lower()}]" if version is not Version.MAIN else ""
return f"{dtype.__name__}{v}"
repr_dtype_class(nw.Datetime, Version.MAIN), repr_dtype_class(nw.Datetime, Version.V1)('Datetime', 'Datetime[v1]')Note
I'm open to changes, this was just the first idea I thought of
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.
('Datetime', 'Datetime[v1]')
I kinda like this - Should this differentiation apply only to new types or all of them when calling from a non-main version?
@MarcoGorelli gentle summoning to get your thoughts as well 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.
Should this differentiation apply only to new types or all of them when calling from a non-main version?
Ah yeah that is a quirk I suppose
I used Version in (#3213 (comment)) because it was how I'd done it before, but I suppose we haven't defined it on DTypes.
And everything besides these guys are just re-exports of main:
from narwhals.stable import v1
v1.Datetime
Datetime[v1]
v1.Duration
Duration[v1]
v1.Enum
Enum[v1]I think the most important part is being able to tell the difference between Enum and v1.Enum, since only one has a constructor.
For everything else, I can see the argument for consistency - but we would be adding noise for things like:
import narwhals as nw
from narwhals.stable import v2
nw.List
List
v1.List
List[v1]
v2.List
List[v2]Or if we extended the idea to DType instances (totally optional):
nw.List(nw.String)
List(String)
v1.List(v1.String)
List[v1](String[v1])
v2.List(v2.String)
List[v2](String[v2])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.
Co-authored-by: Dan Redding <[email protected]>
What type of PR is this? (check all applicable)
Related issues
DType.__repr__and simplify__slots__maintenance via metaclassesΒ #3212Checklist
If you have comments or can explain your changes, please do so below