chore: refactor EagerWhen._if_then_else#2662
chore: refactor EagerWhen._if_then_else#2662MarcoGorelli merged 14 commits intonarwhals-dev:mainfrom
EagerWhen._if_then_else#2662Conversation
| def when( | ||
| self, predicate: EagerExprT | ||
| ) -> EagerWhen[EagerDataFrameT, EagerSeriesT, EagerExprT, NativeSeriesT]: ... | ||
| ) -> EagerWhen[EagerDataFrameT, EagerSeriesT, EagerExprT]: ... |
There was a problem hiding this comment.
I'm struggling to follow this
If I remove NativeSeriesT from EagerWhen, and hence from the return type, then mypy / pyright tell me:
/home/runner/work/narwhals/narwhals/narwhals/_compliant/namespace.py:133:7 - warning: Type variable "NativeSeriesT" used in generic Protocol "EagerNamespace" should be covariant (reportInvalidTypeVarUse)
But, I can't make it covariant, because it's used as an argument to from_native. If I did, type checkers would complain. I can't tell what they're expecting me to do 😄
@dangotbanned sorry for the ping, any tips would be appreciated 🙏
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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
|
I think we may just need to remove One trick could be to add |
| @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: |
There was a problem hiding this comment.
I don't think this should have been merged
| def _gather(self, rows: SizedMultiIndexSelector[Any]) -> Self: ... | ||
| def _gather_slice(self, rows: _SliceIndex | range) -> Self: ... | ||
| def _select_multi_index( | ||
| self, columns: SizedMultiIndexSelector[NativeSeriesT] | ||
| ) -> Self: ... | ||
| def _select_multi_name( | ||
| self, columns: SizedMultiNameSelector[NativeSeriesT] | ||
| ) -> Self: ... | ||
| def _select_multi_index(self, columns: SizedMultiIndexSelector[Any]) -> Self: ... | ||
| def _select_multi_name(self, columns: SizedMultiNameSelector[Any]) -> Self: ... | ||
| def _select_slice_index(self, columns: _SliceIndex | range) -> Self: ... |
There was a problem hiding this comment.
Yeah can you revert this PR?
There was a problem hiding this comment.
A refactor on when shouldn't have to remove typing from other parts of narwhals
2 participants
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