feat: allow passing Series to Series.__getitem__ #1533
Conversation
| ) -> Any | Self: | ||
| if isinstance(item, (int, slice, Sequence)): | ||
| return self._from_native_object(self._native_series.__getitem__(item)) | ||
| return self._from_native_object(self._native_series.__getitem__(item.to_numpy())) |
There was a problem hiding this comment.
🤔 not sure we should be converting to numpy here
I'm not totally sure what we should be doing instead though, sorry, will need to investigate
There was a problem hiding this comment.
Thank you, I had doubts about this too and picked a solution that seemed to work without actually understanding the consequences.
There was a problem hiding this comment.
It seems using item.to_native() instead works, and also solves an issue with missing values becoming NaN with to_numpy(). Do you think it's acceptable? Any other corner cases to cover with tests?
There was a problem hiding this comment.
thanks - i'll check, to be careful we get the details right here
| spl = pl.Series([1, 2, 3]) | ||
| assert spl[spl[0, 1]].equals(pl.Series([2, 3])) |
There was a problem hiding this comment.
can we check what happens when there's missing values too please?
There was a problem hiding this comment.
Using to_native() instead of to_numpy() allows the following tests to pass, but my complete lack of knowledge on numpy, narwhals, polars and related subjects makes me unsure they are correct and show expected behavior:
spl = pl.Series([1, None, 3])
assert spl[spl[0, 1]].equals(pl.Series([None, None]))
snw = nw.from_native(spl, series_only=True)
assert snw[snw[0, 1]].to_native().equals(pl.Series([None, None]))What's your take on this?
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
This PR allows passing
SeriestoSeries.__getitem__and adds a test. I'm not sure about how/where to document the changes, any suggestions?