-
Notifications
You must be signed in to change notification settings - Fork 176
refactor: Simplify calculate_timestamp_datetime and calculate_timestamp_date
#2838
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
refactor: Simplify calculate_timestamp_datetime and calculate_timestamp_date
#2838
Conversation
| def calculate_timestamp_datetime( # noqa: C901, PLR0912 | ||
| s: NativeSeriesT, original_time_unit: str, time_unit: str | ||
| _TIMESTAMP_DATETIME_OP_FACTOR: Mapping[ | ||
| tuple[UnitCurrent, UnitTarget], tuple[BinOpBroadcast, IntoRhs] |
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 the issue is that BinOpBroadcast is generic, but using a TypeVar with a default.
So you don't get a warning when using BinOpBroadcast without specialising - but it causes an issue later when the default conflicts with the constraints of the function signature.
Hopefully this works, I'm replying from my phone π
| tuple[UnitCurrent, UnitTarget], tuple[BinOpBroadcast, IntoRhs] | |
| tuple[UnitCurrent, UnitTarget], tuple[BinOpBroadcast[NativeSeriesT], IntoRhs] |
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.
narwhals/_pandas_like/utils.py:583: error: Argument 1 has incompatible type "_CuDFSeries"; expected "Series[Any]" [arg-type]
narwhals/_pandas_like/utils.py:583: error: Argument 1 has incompatible type "_ModinSeries"; expected "Series[Any]" [arg-type]narwhals/narwhals/_pandas_like/typing.py
Lines 25 to 31 in bb03703
| NativeSeriesT = TypeVar( | |
| "NativeSeriesT", | |
| "pd.Series[Any]", | |
| "_CuDFSeries", | |
| "_ModinSeries", | |
| default="pd.Series[Any]", | |
| ) |
NativeSeriesT means exactly one of pd.Series[Any], _CuDFSeries, _ModinSeries, but default to pd.Series[Any] when not provided.
So the conflict is that
NativeSeriesTis still used for the function parameter and return type- But
_TIMESTAMP_DATETIME_OP_FACTORis currently saying it only acceptspd.Series[Any]
narwhals/narwhals/_pandas_like/utils.py
Lines 576 to 578 in 0e6def9
| def calculate_timestamp_datetime( | |
| s: NativeSeriesT, current: TimeUnit, time_unit: TimeUnit | |
| ) -> NativeSeriesT: |
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.
Hey @dangotbanned thanks for investigating this.
But _TIMESTAMP_DATETIME_OP_FACTOR is currently saying it only accepts pd.Series[Any]
Why would that be the case? The definition is:
BinOpBroadcast: TypeAlias = Callable[[NativeSeriesT, int], NativeSeriesT]
Regarding #2838 (comment), BinOpBroadcast is not a generic, and it would result in:
Type variable "NativeSeriesT" has no meaning in this contextPylance[reportGeneralTypeIssues]
π§
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.
Hey @dangotbanned thanks for investigating this.
But _TIMESTAMP_DATETIME_OP_FACTOR is currently saying it only accepts pd.Series[Any]
Why would that be the case? The definition is:
BinOpBroadcast: TypeAlias = Callable[[NativeSeriesT, int], NativeSeriesT]Regarding #2838 (comment),
BinOpBroadcastis not a generic, and it would result in:
BinOpBroadcast is a generic TypeAlias because it uses a TypeVar (NativeSeriesT) in it's definition.
If we compare that to when you hover over the current use, it uses the default type - because it wasn't subscribed
Type variable "NativeSeriesT" has no meaning in this contextPylance[reportGeneralTypeIssues]
π§
Ahh, so that message it about scoping - not that the TypeVar can't be used at all
So this is because _TIMESTAMP_DATETIME_OP_FACTOR is in a global scope, but we're trying to solve the TypeVar in a function scope
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've just used Any instead in (bdb46aa)
The main reason I had that typing in _arrow was because the stubs caused too many false positives when I used the functions directly.
Here, we're just using operator functions, which are mostly typed with Any anyway π
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.
We still have NativeSeriesT preserved on the outside, so this is simpler than refactoring everything to provide a scope for the TypeVar π
dangotbanned
left a comment
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.
Thanks @FBruzzesi
|
Thank you @dangotbanned π |


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
Heavily inspired by @dangotbanned code for arrow