-
Notifications
You must be signed in to change notification settings - Fork 915
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
Allow None
when nan_as_null=False
in column constructor
#15709
Conversation
None
when nan_as_null=False
None
when nan_as_null=False
in column constructor
elif nan_as_null is False and ( | ||
pd.isna(arbitrary).any() | ||
and inferred_dtype not in ("decimal", "empty") | ||
and inferred_dtype not in ("decimal", "empty", "string") | ||
): | ||
# Decimal can hold float("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.
@mroeschke After the changes in this PR, do you think this block might be redundant or does it still capture some error scenarios?
Co-authored-by: Bradley Dice <[email protected]>
Co-authored-by: Bradley Dice <[email protected]>
pd.isna(arbitrary).any() | ||
any( | ||
(isinstance(x, (np.floating, float)) and np.isnan(x)) | ||
or (inferred_dtype == "boolean" and pd.isna(arbitrary)) |
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.
Would be good to have (inferred_dtype == "boolean" and pd.isna(arbitrary))
be evaluated outside the loop.
Also what case is this condition trying to catch?
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.
Done.
Also what case is this condition trying to catch?
It is trying to catch this case:
pd.Series(["a", "b", np.nan], dtype='object')
f"Cannot have mixed values with {inferred_dtype}" | ||
) | ||
elif ( | ||
nan_as_null is False |
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.
Don't compare to booleans with is
.
nan_as_null is False | |
not nan_as_null |
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.
raise MixedTypeError( | ||
f"Cannot have mixed values with {inferred_dtype}" | ||
) | ||
elif nan_as_null is False and _has_any_nan(arbitrary): |
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.
elif nan_as_null is False and _has_any_nan(arbitrary): | |
elif not nan_as_null and _has_any_nan(arbitrary): |
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 need this comparison because None
is also a supported parameter and it is similar to True
behavior.
Co-authored-by: Bradley Dice <[email protected]>
/merge |
Description
Fixes: #15708
This PR fixes an issue where we were throwing an error when
None
is present andnan_as_null=False
, this is a bug because of usingpd.isna
, this returnsTrue
fornan
,None
andNA
. Whereas we are only looking fornp.nan
and notNone
andpd.NA
Checklist