refactor: Simplify PandasLikeNamespace.concat#2368
Conversation
- More discoverable as related methods - Aligns a bit closer to https://github.com/pola-rs/polars/blob/ecfb7b5e09fd074ede5bd270453237d982495328/py-polars/polars/functions/eager.py#L241
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @dangotbanned 🚀 I like this version a lot!
I might have spotted something in the type annotation?!
narwhals/_pandas_like/namespace.py
Outdated
| return concat(dfs, axis=0, copy=False) | ||
| return concat(dfs, axis=0) | ||
|
|
||
| def _concat_horizontal(self, dfs: Sequence[NDFrameT], /) -> NDFrameT: |
There was a problem hiding this comment.
Shouldn't the output type always be pd.DataFrame regardless of input in dfs?
| def _concat_horizontal(self, dfs: Sequence[NDFrameT], /) -> NDFrameT: | |
| def _concat_horizontal(self, dfs: Sequence[NDFrameT], /) -> pd.DataFrame: |
Here a snippet:
import pandas as pd
s1 = pd.Series([1, 2, 3])
s2 = pd.Series(["a", "b", "c"])
type(pd.concat([s1], axis=1))
# pandas.core.frame.DataFrame
type(pd.concat([s1, s2], axis=1))
# pandas.core.frame.DataFrameThere was a problem hiding this comment.
Well spotted!
So I added that type for horizontal, which gets used with a Series as well (IIRC in group_by maybe?).
vertical used some property that is only available on DataFrame, so I had to make that one narrower.
diagonal (I think) should work with either type, but is currently only used for DataFrame.
Replying from my phone, hope that all makes sense 😅
There was a problem hiding this comment.
Oh I see what you mean now!
There was a problem hiding this comment.
#2368 (comment)
@FBruzzesi I think you'd also need to change the type of dfs to be:
Sequence[pd.DataFrame] | Sequence[pd.Series[Any]]If you kept the TypeVar in pyright will tell you off 😉
Edit: I forgot about this new thing I learned 🤦♂️ #2283 (comment)
There was a problem hiding this comment.
Makes it easier to spot when patterns can be moved up a level in the protocol
| if self._implementation.is_pandas() and self._backend_version < (3,): | ||
| if self._backend_version < (1,): | ||
| return self._concat(dfs, axis=VERTICAL, copy=False, sort=False) | ||
| return self._concat(dfs, axis=VERTICAL, copy=False) | ||
| return self._concat(dfs, axis=VERTICAL) |
There was a problem hiding this comment.
@MarcoGorelli should _concat_vertical be matching this?
I just copied it over, but they have a subtle difference:
narwhals/narwhals/_pandas_like/namespace.py
Lines 273 to 275 in 657aef9
There was a problem hiding this comment.
probably fine for now, perhaps let's make an issue and revisit?
* refactor: refactor: Simplify `ArrowNamespace.concat` Related #2368 * chore(typing): Ignore stub issues Fixed in zen-xu/pyarrow-stubs#203 * cov https://github.com/narwhals-dev/narwhals/actions/runs/14422362924/job/40446526457?pr=2381 * refactor: Implement `EagerNamespace.concat` * refactor: Avoid redundant unique check Has no coverage, would be caught later with a nicer error anyway w/ https://github.com/narwhals-dev/narwhals/blob/41871f775589359c563150526d03935449709d7d/narwhals/utils.py#L1466-L1475 * refactor: Consistently use `chain.from_iterable` * chore: Remove notes One became a type, the other is moving to github
3 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
horizontal_concatvertical_concatdiagonal_concat