-
Notifications
You must be signed in to change notification settings - Fork 87
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: Add 'IntoSeries' #991
Conversation
c2678c5
to
73f46e1
Compare
This reverts commit 94a1bd1.
for more information, see https://pre-commit.ci
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 @luke396 !
sorry this took me a while to get to, I had a company retreat + holiday recently so had relatively little time
looks good, just got a little question
Should we miss some questions here? |
narwhals/typing.py
Outdated
IntoSeries: TypeAlias = Union[ | ||
"Series", "DataFrame[Any]", "LazyFrame[Any]", "NativeSeries" | ||
] |
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.
sorry, the question didn't go for some reason
I wanted to ask, why include "DataFrame[Any]", "LazyFrame[Any]"
here?
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.
I don't recall any particular reason; I just defined it to pass all checks and seem to have forgotten to restrict it again.
Let's try how about remove them.
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.
Seems fine!
@@ -738,7 +739,7 @@ def from_native( | |||
|
|||
@overload | |||
def from_native( | |||
native_dataframe: Any, | |||
native_dataframe: IntoSeriesT | Any, # remain `Any` for downstream compatibility |
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.
def _from_array_like(obj: Iterable[Any], /) -> list[Any]:
try:
ser = nw.from_native(obj, strict=True, series_only=True)
return ser.to_list()
except TypeError:
return list(obj)
narwhals/typing.py
Outdated
class NativeSeries(Protocol): | ||
@property | ||
def name(self) -> Any: ... |
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.
not sure this is true in general unfortunately π
In [4]: pa.chunked_array([['a', 'b', 'c']]).name
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[4], line 1
----> 1 pa.chunked_array([['a', 'b', 'c']]).name
AttributeError: 'pyarrow.lib.ChunkedArray' object has no attribute 'name'
let's just put __len__
?
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.
Thank you for the correction. I didn't consider the situation.
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 @luke396 !
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.
I think I should keep learning from #365 to improve the current PR.