-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
BUG: fix IntegerArray astype with copy=True/False #34931
Conversation
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.
LGTM. The matplotlib test failure is unrelated and I think reported elsewhere.
Does this need a whatsnew, or is it only on master?
One additional API question that pops up: in numpy, |
Not only master it seems, will add a whatsnew. |
I don't have a strong opinion on returning a new object backed by the data vs. just returning |
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.
lgtm very minor comment
------- | ||
type | ||
""" | ||
raise NotImplementedError |
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.
should use AbstractMethodError
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.
We actually use NotImplementedError in the base dtype class for this, so this is consistent with that (not fully sure why we do that though, there might be a reason)
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.
kk yeah we should actually change that as this is a better error message (but ok)
Any preferences on what I do with |
Matching NumPy (returning the same object) seems OK. |
Updated to return same object |
lgtm, does this need a whatsnew? e.g. changed in 1.1? |
Added a bug fix note |
thanks @jorisvandenbossche |
xref #34307 (comment)
Right now, we were not copying the mask correctly with
copy=True
, and also not when the data was actually copied (then the mask also needs to be copied, even withcopy=False
)