TYP: remove a type: ignore
#44288
TYP: remove a type: ignore
#44288
Conversation
| @@ -462,7 +462,7 @@ def find( | |||
| dtype_type = type(dtype) | |||
| else: | |||
| dtype_type = dtype | |||
| if issubclass(dtype_type, ExtensionDtype): | |||
| if dtype_type in self.dtypes: | |||
There was a problem hiding this comment.
self.dtypes is actually a list. i am not 100% sure this matters but can you check (e.g. perf)
There was a problem hiding this comment.
yes, calling find with a EA instance or EA type maybe slower if we actually check the registry which I would expect a method called find to do.
but looking some more I think we potentially have another issue.
we call Registry.find from pandas_dtype.
So the immediate thoughts are
- pandas.api.types.pandas_dtype is public so any changes need extra scrutiny.
- pandas_dtype is annotated and documented as returning a dtype instance whereas Registry.find may return an instance or a Type.
- pandas_dtype accepts any object and may pass that onto Registry.find so the annotations for Registry.find may need modification.
will mark as draft for now to prevent merging and try to filter type annotations for functions calling pandas_dtype to lower levels before looking at the perf implications.
>>> import pandas as pd
>>> from pandas.api.types import pandas_dtype
>>>
>>> pandas_dtype("Int64")
Int64Dtype()
>>>
>>> pandas_dtype(pd.Int64Dtype)
<class 'pandas.core.arrays.integer.Int64Dtype'>
according to the docs and annotations (point 2), this last statement should return an Int64Dtype instance
I think it maybe better that Registry.find only accepts strings and the other logic for EA types and instances is included in is_extension_array and pandas_dtype. will investigate further.
There was a problem hiding this comment.
pandas_dtype is annotated and documented as returning a dtype instance whereas Registry.find may return an instance or a Type.
FWIW i think changing this to always return a dtype instance would fix #31356
|
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
|
status here? |
|
Thanks for the PR, but it appears to have gone stale. Feel free to reopen when you have time to revisit. |
No description provided.