-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Allow numeric ExtensionDtypes in DataFrame.select_dtypes #38246
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
Conversation
pandas/core/frame.py
Outdated
if issubclass( | ||
unique_dtype.type, tuple(dtypes_set) # type: ignore[arg-type] | ||
) | ||
if np_issubclass_compat(unique_dtype, dtypes_set) |
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.
i don't think we need to create yet another method here. is there a reason you cannot just use something like
return issubclass(unique_dtype.type, tuple(dtypes_set)) or (
np.number in dtypes_set and is_numeric_dtype(unique_dtype))
)
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.
no reason. Done
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.
Having now tried this I think the reason might be mypy
pandas/core/frame.py:3715: error: Argument 1 to "tuple" has incompatible type "FrozenSet[Union[ExtensionDtype, str, Any, Type[str], Type[float], Type[int], Type[complex], Type[bool], Type[object]]]"; expected "Iterable[Union[type, Tuple[Any, ...]]]" [arg-type]
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.
Reverted to having a separate 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.
just ignore mypy here, this makes groking the code way more complicated.
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!
pandas/core/frame.py
Outdated
) | ||
or ( | ||
np.number in dtypes_set | ||
and hasattr(unique_dtype, "_is_numeric") # is an extensionarray |
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 u check is_extension_array_dtype here instead
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
Green + addressed comments |
pandas/core/frame.py
Outdated
or ( | ||
np.number in dtypes_set | ||
and is_extension_array_dtype(unique_dtype) | ||
and unique_dtype._is_numeric |
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.
use getattr(unique_dtype, '_is_numeric', False)
as we don't actually require this on an EA type (maybe we should though)
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
is_selected = df.select_dtypes(np.number).shape == df.shape | ||
assert is_selected == expected | ||
|
||
# da = DummyArray([1, 2], dtype=DummyDtype(numeric=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.
remove commented code
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
Green + addressed comments |
thanks @arw2019 |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Picking up #35341 (original PR addressed all comments - here I merged master + added whatsnew note)
cc @simonjayhawkins @jreback @jbrockmendel