chore: Track ExpansionKind in ExprMetadata#2266
chore: Track ExpansionKind in ExprMetadata#2266dangotbanned merged 23 commits intonarwhals-dev:mainfrom
ExpansionKind in ExprMetadata#2266Conversation
MarcoGorelli
force-pushed
the
expansion-kind
branch
from
4b507ad to
f7f0a90
Compare
| ) | ||
|
|
||
|
|
||
| def resolve_expansion_kind(lhs: ExpansionKind, rhs: ExpansionKind) -> ExpansionKind: |
There was a problem hiding this comment.
Maybe make this a method on either of these?
ExpansionKindExprMetadataExpr
`from_column_indices` was removed without reason?
Was broken for me locally following narwhals-dev#2263 (comment)
| nw.all().sum(). | ||
| """ | ||
| assert self._metadata is not None # noqa: S101 | ||
| return self._metadata.expansion_kind.is_multi_unnamed() |
There was a problem hiding this comment.
(#2266 (comment)) can be taken a step further by adding:
ExprMetadata.is_multi_unnamedSo this part wouldn't need to even know what an ExpansionKind is, and having the line here be:
return self._metadata.is_multi_unnamed()But this works as well
|
@MarcoGorelli I'm not sure on the merge conflicts |
| Elementary expressions are the only ones supported properly in | ||
| pandas, PyArrow, and Dask. |
There was a problem hiding this comment.
I copied directly from is_elementary_expression - but this line might be redundant now?
Maybe a description in the class doc could have some stuff from https://narwhals-dev.github.io/narwhals/how_it_works
Forgot to remove this
sure, i'll resolve |
|
thanks a tonne for your help - happy to ship this if you're on-board? |
dangotbanned
left a comment
dangotbanned
left a comment
There was a problem hiding this comment.
thanks a tonne for your help - happy to ship this if you're on-board?
Ofc @MarcoGorelli, thanks for working on this and being so receptive to feedback
dangotbanned
changed the title
ExpansionKind in ExprMetadata
|
@MarcoGorelli Let me know if you want me to fix the conflicts following #2261 |
|
sure, if you don't mind, thanks! |
| _call: Callable[[FrameT], Sequence[SeriesT]] | ||
| _when_value: CompliantWhen[FrameT, SeriesT, ExprT] | ||
| _function_name: str | ||
| _depth: int |
There was a problem hiding this comment.
Minor nuisance, can fix properly in a follow-up and align DaskWhen with the current EagerWhen as DepthTrackingWhen
2 participants
ref #2225
I think that to properly close the issue, a section to "how it works" should be added
But this at least deals with the internals and gets rid of a hack I was really not proud of (behaviour based on
function_name). There is still a bit of that, but we're on the path to removing itWhat type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below