Skip to content
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.

Sign up for GitHub

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

BUG: Series.__getitem__ with downstream scalars #32684

Merged
merged 6 commits into from
Mar 19, 2020
Merged

BUG: Series.__getitem__ with downstream scalars #32684

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
29bc817
loosen scalar check in Series.__getitem__
jbrockmendel Mar 13, 2020
75442c9
Merge branch 'master' of https://github.com/pandas-dev/pandas into fi…
jbrockmendel Mar 15, 2020
efba858
Merge branch 'master' of https://github.com/pandas-dev/pandas into fi…
jbrockmendel Mar 18, 2020
b982835
test cases
jbrockmendel Mar 18, 2020
b4fbb2a
check differently, whatsnew, test
jbrockmendel Mar 18, 2020
b6bf039
remove unnecessary call, whatsnew but this time in the correct branch
jbrockmendel Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion pandas/core/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,11 +840,15 @@ def _slice(self, slobj: slice, axis: int = 0) -> "Series":

def __getitem__(self, key):
key = com.apply_if_callable(key, self)
key = lib.item_from_zerodim(key)

if key is Ellipsis:
return self

key_is_scalar = is_scalar(key)
# check for is_list_like/slice instead of is_scalar to allow non-standard
# scalars through, e.g. cftime.datetime needed by xarray
# https://github.com/pydata/xarray/issues/3751
key_is_scalar = not is_list_like(key) and not isinstance(key, slice)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really really sketch, why don't we just fix is_scalar?

also pls add a test if you can for this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just fix is_scalar?

Depends on what we want is_scalar to mean. We could re-write is_scalar to just match this (actually more performant than what we have now), but it isn't really viable to add checks for any custom scalar that downstream libraries might implement.

Going to wait to hear from the downstream folks on if this actually solves their problem(s)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think is_scalar should just be this its much more generic and future proof

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm so changing is_scalar to just match this is breaking a bunch of tests bc is_scalar(some_lambda_func) is returning True

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right you might need some more exclusions, e.g. callabes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the trouble im facing ATM is that TimeGrouper is being recognized as a scalar

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I look at this the more skeptical I am of trying to amend is_scalar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we don't actually have a test case for this, am inclined to close.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coming up with a test case isnt difficult, just need to settle on an approach.

if isinstance(key, (list, tuple)):
key = unpack_1tuple(key)

Expand Down