-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat: Series descriptive methods #187
Conversation
utils/check_api_reference.py
Outdated
.difference( | ||
{"to_pandas", "to_numpy", "dtype", "name", "shape", "to_frame", "is_empty"} | ||
) |
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.
Would it make sense to dynamically extract the set of methods that a polars Series supports but Expr does not? Something like the following should be enough:
series_only = [
i for i in dir(pl.Series)
if not i[0].isupper() and i[0] != "_" and i not in dir(pl.Expr)
]
Let me know what you think
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.
maybe, but there's also value in this being manual, so we can inspect whether it makes sense that a given method is missing
thanks @FBruzzesi ! Looks like |
Thanks Marco, it seems that both naming and order is different (as if |
I think order is fine, there are other places where the order isn't preserved (e.g. group_by) - shall we just document the lack of ordering guarantee, and fixup the names? |
Pandas |
yeah, sorry, hadn't looked carefully enough - sure, this one should be easy enough to work around and manually fixup |
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.
Wow, absolute legend mate!
I can never remember the is_first_distinct
in pandas, so I may need to refer to this if I ever need to write pandas again (and I probably will at some point 😄 )
Just made a minor comment
Approving, though I didn't sleep much tonight (got up early to catch a train) so I'm not super-"with it", feel free to merge if you're confident
narwhals/_pandas_like/series.py
Outdated
"""Parallel is unused, exists for compatibility""" | ||
from narwhals._pandas_like.dataframe import PandasDataFrame | ||
|
||
name_ = self._series.name or "index" |
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.
if self._series.name
is something like 0
or ''
, will this overwrite it with 'index'
? should be specifically check for if self._series.name is None
?
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! That's a great catch 🙈
Description
Similar to #159 but for series and expr methods (complete list from polars); introduces the following Series methods: