-
Notifications
You must be signed in to change notification settings - Fork 180
chore: refactor EagerWhen._if_then_else
#2662
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 all commits
d708aeb
8dfc79a
bad8fed
29d6258
aab3a70
cdf57d9
3c7eb58
c8b4f0a
c4974f7
5a5f7a7
20793f5
e19d5e1
e7787bd
1d36fbc
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 |
|---|---|---|
|
|
@@ -23,7 +23,6 @@ | |
| LazyExprT, | ||
| NativeFrameT, | ||
| NativeFrameT_co, | ||
| NativeSeriesT, | ||
| ) | ||
| from narwhals._utils import ( | ||
| exclude_column_names, | ||
|
|
@@ -132,31 +131,17 @@ def from_native(self, data: NativeFrameT_co | Any, /) -> CompliantLazyFrameT: | |
|
|
||
| class EagerNamespace( | ||
| DepthTrackingNamespace[EagerDataFrameT, EagerExprT], | ||
| Protocol[EagerDataFrameT, EagerSeriesT, EagerExprT, NativeFrameT, NativeSeriesT], | ||
| Protocol[EagerDataFrameT, EagerSeriesT, EagerExprT, NativeFrameT], | ||
| ): | ||
| @property | ||
| def _dataframe(self) -> type[EagerDataFrameT]: ... | ||
| @property | ||
| def _series(self) -> type[EagerSeriesT]: ... | ||
| def when( | ||
| self, predicate: EagerExprT | ||
| ) -> EagerWhen[EagerDataFrameT, EagerSeriesT, EagerExprT, NativeSeriesT]: ... | ||
| ) -> EagerWhen[EagerDataFrameT, EagerSeriesT, EagerExprT]: ... | ||
|
Comment on lines
143
to
+142
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. I'm struggling to follow this If I remove But, I can't make it covariant, because it's used as an argument to @dangotbanned sorry for the ping, any tips would be appreciated π
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. Hey @MarcoGorelli, I'll try to take a look at this today π Would you be able to give some more background on what you're trying to accomplish with this PR please? I'm not following the motivation from either the title or commit messages π€
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. sure, it's to enable #2652 and #2645 i spent about 2 hours trying different things and wasn't able to get anywhere with this π³ , based on #2662 (comment) it looks to me like there were already issues with these protocols. happy for some parts to be brought back, so long as we're able to move towards the linked issues |
||
|
|
||
| @overload | ||
| def from_native(self, data: NativeFrameT, /) -> EagerDataFrameT: ... | ||
| @overload | ||
| def from_native(self, data: NativeSeriesT, /) -> EagerSeriesT: ... | ||
| # TODO @dangotbanned: Align `PandasLike` typing with `_namespace`, then drop this `@overload` | ||
| # - Using the guards there introduces `_NativeModin`, `_NativeCuDF` | ||
| # - These types haven't been integrated into the backend | ||
| # - Most of the `pandas` stuff is still untyped | ||
| @overload | ||
| def from_native( | ||
| self, data: NativeFrameT | NativeSeriesT | Any, / | ||
| ) -> EagerDataFrameT | EagerSeriesT: ... | ||
| def from_native( | ||
| self, data: NativeFrameT | NativeSeriesT | Any, / | ||
| ) -> EagerDataFrameT | EagerSeriesT: | ||
|
Comment on lines
-145
to
-159
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 don't think this should have been merged |
||
| def from_native(self, data: Any, /) -> EagerDataFrameT | EagerSeriesT: | ||
| if self._dataframe._is_native(data): | ||
| return self._dataframe.from_native(data, context=self) | ||
| elif self._series._is_native(data): | ||
|
|
||
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.
Yeah can you revert this PR?
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.
A refactor on
whenshouldn't have to remove typing from other parts ofnarwhals