BUG: Series.getitem with downstream scalars by jbrockmendel · Pull Request #32684 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits6 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes #xxxx
- tests added / passed
- passes
black pandas - passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
cc @spencerclark I think this fixes a subset of the issues reported in pydata/xarray#3751, can you confirm?
@jorisvandenbossche IIRC geopandas scalars not being recognized by lib.is_scalar has caused some issues there; does this address any of those?
| # 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) |
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
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)
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
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
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
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
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
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.
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.
Related to indexing on series/frames, not to indexes themselves
label
Thanks @jbrockmendel -- sorry I just saw this now. Yes, as written, this change indeed fixes the remaining issue for us, though I think @jreback's suggestion would work too.
Updated with a better implementation, and a test that fails in master: df.dtypes[some_dtype] currently raises, is fixed here.
SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request
jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request
jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request
Labels
Related to indexing on series/frames, not to indexes themselves