TST: simplify pyarrow tests, make mode work with temporal dtypes #50688
Conversation
| def test_getitem_scalar(self, data): | ||
| super().test_getitem_scalar(data) | ||
| # In the base class we expect data.dtype.type; but this (intentionally) |
There was a problem hiding this comment.
@mroeschke might it make sense to reconsider this? im not sure what context the current PyArrow.lib.DataType is really useful
There was a problem hiding this comment.
Sure, yeah I am not exactly sure how ExtensionDtype.type is used internally, but I think it would make sense if it returns the native python type. Would be nice if there was a nice way to derive that from numpy_dtype
There was a problem hiding this comment.
the main use case that comes to mind is if i want to do e.g. dtype.type(0) to get an object of the correct type
pandas/core/arrays/arrow/array.py
Outdated
| if pa.types.is_temporal(pa_type): | ||
| nbits = pa_type.bit_width | ||
| dtype = f"int{nbits}[pyarrow]" | ||
| obj = cast(ArrowExtensionArrayT, self.astype(dtype, copy=False)) |
There was a problem hiding this comment.
Can we just self._data.cast to int type, continue the logic below, and then most_common.cast back?
There was a problem hiding this comment.
we could. i preferred this way since it was self-contained
There was a problem hiding this comment.
I suspect going through pa.array.cast may be more performant than astype. Mind checking?
There was a problem hiding this comment.
Looks like _mode here is still using astype
There was a problem hiding this comment.
yes, in some cases this needs an int32 and in some cases an int64, which makes the .cast version extra-complicated
mroeschke
added
Testing
|
If I am understanding apache/arrow#33685, then in pyarrow 11 we won't have to do the the int64 workaround? |
I don't know if that will affect mode directly. |
|
updated to use .cast instead of .astype |
|
Thanks @jbrockmendel |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.