Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
API: membership checks on ExtensionArray containing NA values #37867
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
API: membership checks on ExtensionArray containing NA values #37867
Changes from 14 commits
7ab6443
7986bc4
466f7cc
a9b75dd
9d0eca1
b0b32ab
c6e42d2
75c45bc
83c9fe4
08c4c98
5a23b1d
4b0c200
92604e9
8a24f0d
fdb9deb
52e2b43
f21890e
6f633c7
d8bdb2e
4e4dbc4
a1583e7
3c2c2b0
237fe45
37219c3
c4a6c36
245c99a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
can you use
self.isna()
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.
This could maybe be done in a separate PR, as it is FloatingArray specific (and the NaN behaviour is not yet fully fleshed out), but this check will do the wrong thing for
np.nan
if the FloatingArray actually contains a NaN (asself.dtype._na_value
is then pd.NA and not np.nan).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.
So a
FloatingArray
can have multiple nan types? Is that really necessary?I think I would prefer that would be handled in
FloatingArray
, so I don't have to stretch this PR too far.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 multiple "nan types", to be nitpicky, since pd.NA is not a "nan". But yes, it can contains both pd.NA and np.nan at the moment. But that's not yet fully decided though, long discussion at #32265. Happy to hear your thoughts about it.
Yes, will put it on my list of follow to-do items for FloatingArray that I need to open. So don't worry about it 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.
data
is not guaranteed to have a missing values, but there is also adata_missing
fixture that has this guarantee and which you can add here to the test, and use that here.And then to be sure we test the case of data without NAs, maybe do the else with
data[~data.isna()]
(that is guaranteed to not be empty)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.
And thanks for this base test!
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.
Ok, I've added a test method using data_missing.
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.
BTW, you can pass both fixtures to the same test, then you can write the test with less duplication as it is now
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.
you can pass them as the string name of the test and use:
frame = request.getfixturevalue(fixture_func_name)
to create the actual frameThere 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.
is than issue number for this?
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.
The issue number is the first line in the method?
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.
in the xfail pls
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.
ok.