BUG/TST: Fix infer_dtype for Period array-likes and general ExtensionArrays
State: closed

Closes #23553

In addition, I also changed to let infer_dtype fall back to inferring from the scalar elements if the array-like is not recognized directly (now it raises an error, which doesn't seem very useful?)

Labels:
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

Comments:
@jorisvandenbossche if you can merge master and update for comments
> random python objects will be marked as 'mixed' in any event w/o the performance panelty below.

We could also simply return "mixed" as a kind of "unknown" instead of converting to a numpy array (or instead of the original exception). 


So the inconsistency that I thought to solve by removing the exception is this difference:

```
In [1]: from pandas.tests.extension.decimal import DecimalArray, make_data

In [2]: arr = DecimalArray(make_data())

In [3]: pd.api.types.infer_dtype(arr)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-782ef3c80fa8> in <module>
----> 1 pd.api.types.infer_dtype(arr)

pandas/_libs/lib.pyx in pandas._libs.lib.infer_dtype()

pandas/_libs/lib.pyx in pandas._libs.lib._try_infer_map()

AttributeError: 'DecimalDtype' object has no attribute 'base'

In [4]: pd.api.types.infer_dtype(list(arr))
Out[4]: 'decimal'
```

(with the other change, the exact exception would change to "ValueError: cannot infer type for DecimalArray")

Now, of course, when passing a custom EA (unknown to pandas), `infer_dtype` can actually be costly with this PR because it will do a conversion to numpy array (which can be costly for an EA). While before it raised an error before converting to numpy. 

So it might make sense to keep the exception. 
But that in general makes the usage of `infer_dtype` also annoying, as anywhere we use it, it can potentially raise an error (which we currently don't really account for). So returning the fallback "mixed" could also be an option instead of converting to a numpy array, since "mixed" is already something we handle. 
> We could also simply return "mixed" as a kind of "unknown" instead of converting to a numpy array (or instead of the original exception).

I like this idea better than converting to ndarray.  Maybe something other than "mixed" though, so as to keep the meaning of "mixed" unambiguous?
My knee-jerk reaction to the DecimalArray case was "instead of casting to ndarray, we should just add 'decimal' to `_TYPE_MAP`".  But that goes against the "we dont want to special-case our internal EAs any more than we have to" principle.

So two ideas:

1) if we get an EA, can just return `values.dtype.name` or something like that
2) part of the register_dtype process could add stuff to _TYPE_MAP (may just be a more complicated version of 1?)
yeah i think having a good path for EAs is ideal here.
> But that goes against the "we dont want to special-case our internal EAs any more than we have to" principle.

Note that DecimalArray isn't even an internal EA, it's a test case for an external EA (as long as we don't have a proper decimal dtype, at least ;)) 
Because we *are* already special casing our internal EAs here, since those are present in the `_TYPE_MAP` (and which I think is no problem to do here).





So I did a rough search / inventory of the different use cases internally of `infer_dtypes`. The main groups I see:

- Many use cases are to infer a specific subset (eg "string", or "floating"/"integer"/"mixed-integer-float", or "boolean") from a list or object-dtype array
  -> since for those case we know we don't start with array with a specific dtype (except object type), this will never take the EA path
- Infer "period"/"interval"/"datetime"/.. dtype from a non-EA (again not impacted by this discussion)
- Infer "mixed-integer" for sorting (also not impacted by this discussion)
- Infer dtype from an np.ndarray (idem)
- Infer "integer" key type for indexing
  -> here we can potentially pass any EA (once we can use them for indexing), so for this use case it is actually important that `infer_dtype(EA)` doesn't raise an error (and the actual return value then doesn't matter for non-integer EAs)
- ...

So from that, I think that it will actually be **good to change that `infer_dtype(EA)` never raises an error** (as it does now on master for unknown array types).

The question is then which value? Infer by converting to object dtype numpy array, return an existing value "mixed", or return a new value like "unknown-array" ? Or let the EA dtype register something?

Given the potential expensive nature of coercing to object dtype, that might be something to avoid. 
Given that "mixed" is already being used and has some use cases (eg the validation for the str accessor), it might be better to not re-use that. 

> So two ideas:
>
>  1. if we get an EA, can just return values.dtype.name or something like that
>  2. part of the register_dtype process could add stuff to _TYPE_MAP (may just be a more complicated version of 1?)

If we let the EA control this, I think it only can make sense if they return one of the existing categories? (what would we otherwise ever do with it, except ignoring it?) 
So that would rule out the first option, I think? 

Long term, it might be useful to let the dtype register its "inferred_dtype", but then I think we should first have a better idea of some specific use cases for which this would be used/useful (currently, many use case I checked was to do some dtype inference when not yet having an array like to start with). 

So on the short term, maybe we can **use the "unknown-array" return value**? That would also not be used in practice, so it would mean it is basically ignored, but then at least without raising an error. 
> So on the short term, maybe we can use the "unknown-array" return value? That would also not be used in practice, so it would mean it is basically ignored, but then at least without raising an error.

+1
> > So on the short term, maybe we can use the "unknown-array" return value? That would also not be used in practice, so it would mean it is basically ignored, but then at least without raising an error.
> 
> +1

yep aggeed let's do this for now.

Is there a reason we cannot call `registry.find()` right now in this PR?

> Is there a reason we cannot call registry.find() right now in this PR?

What do you mean exactly? 
The registry is to convert a string into a type AFAIK, while here we want to know the general "type" given a dtype
> > Is there a reason we cannot call registry.find() right now in this PR?
> 
> What do you mean exactly?
> The registry is to convert a string into a type AFAIK, while here we want to know the general "type" given a dtype

sure once we have the actual dtype object, then we effectively have what's in _TYPE_MAP
Ah, OK, but so in the case of an ExtensionArray being passed (the case under discussion), we already have a type object
Updated this to return "unknown-array" for ExtensionArrays we don't have in our TYPE_MAP
moving this off 1.2
@jreback is there any compromise available here?  its becoming a blocker for other things
> @jreback is there any compromise available here? its becoming a blocker for other things

I have already given that *any* dtype parsing needs to occur as a method on the Dtype object itself (it IS available), it is a trivial change (e.g. you have a method .base_dtype or whatever which returns 'period' in this case. (which you can then lookup in the TYPE_MAP)
> you have a method .base_dtype

I am not aware of such a method. Is it your suggestion to *add* such a method (or rather attribute, I suppose)? 
yes exactly
Instead of adding a new attribute, doesnt `dtype.name` work in most/all of the existing cases?
In many cases, but not in all. That's the whole point of this PR (apart from adding "period" to the `TYPE_MAP` dict): we already check the dtype's `name`, but that doesn't work for period, and doesn't work in general for parametrized dtypes.

We seem to be a bit inconsistent on whether we include the parametrization in the name of the dtype or not. For example:

```
In [87]: pd.PeriodDtype("D")
Out[87]: period[D]

In [88]: str(pd.PeriodDtype("D"))
Out[88]: 'period[D]'

In [89]: pd.PeriodDtype("D").name
Out[89]: 'period[D]'

In [90]: pd.IntervalDtype(np.int64)
Out[90]: interval[int64]

In [91]: str(pd.IntervalDtype(np.int64))
Out[91]: 'interval[int64]'

In [92]: pd.IntervalDtype(np.int64).name
Out[92]: 'interval'
```

So for Interval dtype, the "name" check already works, for Period not. 

Maybe we should change `PeriodDtype.name`, though. But not directly sure what the impact of that would be.
DatetimeTZ also includes the parametrization in the name:

```
In [95]: pd.DatetimeTZDtype(tz="UTC")
Out[95]: datetime64[ns, UTC]

In [96]: pd.DatetimeTZDtype(tz="UTC").name
Out[96]: 'datetime64[ns, UTC]'
```

(so it might be interval that is the outlier)
> Do we want to import [Interval, Decimal, Period] in lib.pyx ? (might be fine, eg there are already imports from tslibs.period, but at the moment only cimport s)
>
> If that's OK, I am fine with changing it. I don't necessarily find it less "hacky" than the current solution, but I just want some solution that is acceptable for all

We already have Decimal in the namespace, and I think importing Interval and Period would be pretty benign (no circular dependencies).  I'd be happy with this solution.
> In many cases, but not in all. That's the whole point of this PR (apart from adding "period" to the `TYPE_MAP` dict): we already check the dtype's `name`, but that doesn't work for period, and doesn't work in general for parametrized dtypes.
> 
> We seem to be a bit inconsistent on whether we include the parametrization in the name of the dtype or not. For example:
> 
> ```
> In [87]: pd.PeriodDtype("D")
> Out[87]: period[D]
> 
> In [88]: str(pd.PeriodDtype("D"))
> Out[88]: 'period[D]'
> 
> In [89]: pd.PeriodDtype("D").name
> Out[89]: 'period[D]'
> 
> In [90]: pd.IntervalDtype(np.int64)
> Out[90]: interval[int64]
> 
> In [91]: str(pd.IntervalDtype(np.int64))
> Out[91]: 'interval[int64]'
> 
> In [92]: pd.IntervalDtype(np.int64).name
> Out[92]: 'interval'
> ```
> 
> So for Interval dtype, the "name" check already works, for Period not.
> 
> Maybe we should change `PeriodDtype.name`, though. But not directly sure what the impact of that would be.

i c, yeah maybe let's just fix name  to be what i am suggesting as 'base_name'
> i c, yeah maybe let's just fix name to be what i am suggesting as 'base_name'

I am not sure we can "just" fix that. It's not just Period dtype, but basically any parametrized dtype (except for interval) does this. I don't directly know what the impact would be when changing all their `name`s (it's public interface, so it would eg also be a breaking change)
> > i c, yeah maybe let's just fix name to be what i am suggesting as 'base_name'
> 
> I am not sure we can "just" fix that. It's not just Period dtype, but basically any parametrized dtype (except for interval) does this. I don't directly know what the impact would be when changing all their `name`s (it's public interface, so it would eg also be a breaking change)

ok, then let's add `.base_name` for now and open an issue to see if we can normalize `.name` to be equivalent to `str(dtype)`
> ok, then let's add .base_name for now and open an issue to see if we can normalize .name to be equivalent to str(dtype)

maybe `generic_name`?  avoids confusion with `.base.name`

Longer term, we could require that parametrized dtypes have a shared generic `.base` dtype and return `dtype.base.name` here
> > ok, then let's add .base_name for now and open an issue to see if we can normalize .name to be equivalent to str(dtype)
> 
> maybe `generic_name`? avoids confusion with `.base.name`
> 
> Longer term, we could require that parametrized dtypes have a shared generic `.base` dtype and return `dtype.base.name` here

sgtm
I am bit hesitant to start adding new attributes to dtypes just for this (that requires a broader discussion on the different naming attributes of the dtypes IMO). 

But so I implemented the idea of Brock to add Period itself in the type_map, and thus removed the custom "string parsing" block that caused the discussion.
> also the OP has an example for IntervalArray, can you add a test for that as well.

The interval case was already fixed before in https://github.com/pandas-dev/pandas/pull/27653, which also already added specific `infer_dtype` tests for it.