REF/API: Stricter extension checking.
State: closed

Removes is_extension_array_dtype's handling of both arrays and dtypes.
Now it handles just arrays, and we provide `is_extension_dtype` for checking
whether a dtype is an extension dtype. It's the caller's responsibility to know
whether they have an array or dtype.

Closes #22021

Labels:
name: Refactor
description: Internal refactoring of code
name: Build
description: Library building on various platforms
name: API Design

Comments:
Hello @TomAugspurger! Thanks for updating the PR.







Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on July 30, 2018 at 13:09 Hours UTC
> why are you changing this?

To fix #22021

> we do this for all other is_*

Yeah, and that's why `is_extension_array_dtype` originally followed the same pattern. But I think it's not a good idea to pass values to `pandas_dtype`. All the call sites knew whether they had values or a dtype.

I can make similar changes for the other methods, but we have backwards compatibility issues there.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22031?src=pr&el=h1) Report
> Merging [#22031](https://codecov.io/gh/pandas-dev/pandas/pull/22031?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/26b3e7de56fdebaf1842b01d747a1a30a126c54a?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22031/graphs/tree.svg?token=eZ4WkYLtcO&height=150&src=pr&width=650)](https://codecov.io/gh/pandas-dev/pandas/pull/22031?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22031      +/-   ##
==========================================
- Coverage   92.07%   92.07%   -0.01%     
==========================================
  Files         170      170              
  Lines       50688    50684       -4     
==========================================
- Hits        46671    46666       -5     
- Misses       4017     4018       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.48% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.3% <100%> (-0.02%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22031?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/dtypes.py](https://codecov.io/gh/pandas-dev/pandas/pull/22031/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2R0eXBlcy5weQ==) | `96.05% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/22031/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `94.87% <100%> (-0.34%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22031?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22031?src=pr&el=footer). Last update [26b3e7d...a97cec4](https://codecov.io/gh/pandas-dev/pandas/pull/22031?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> Wouldn't it be more logical to call this is_extension_array instead of is_extension_array_dtype? (I suppose the reason is backwards compatibility for at least one of the two use cases?)

Yes. These weren't in the public namespace earlier, so we don't have to worry about deprecating.

> Adding 2 different methods is very confusing

How so? Let's step back to the original issue: `pandas_dtype` shouldn't pass *data* to `np.dtype`. Say I have an object array `arr = [('f1', 'f2'), ('f2', 'f4')]`, as part of an operation that gets sliced to `arr[0]`, and we end up calling `pandas_dtype(subarr)`.

```python
In [28]: subarr = [('f1', 'f2')]

In [29]: pd.api.types.pandas_dtype(subarr)
Out[29]: dtype([('f1', '<f2')])
```

Is that what you would expect?

> we on't do this for anything else and introduces a special case.

As I said,

> I can make similar changes for the other methods, but we have backwards compatibility issues there.

> I can't believe this didn't break anything

Why? As Tom said, "all the call sites knew whether they had values or a dtype.", and there are not yet that many usages in the code base currently for this. 
I personally like the fact we are explicit in the code about whether we are checking a dtype or an array-like.

> Adding 2 different methods is very confusing as we on't do this for anything else and introduces a special case.

Note that we actually have two methods for some other types as well: we have `is_categorical` and `is_categorical_dtype`, `is_datetimetz` and `is_datetime64tz_dtype` (where the first act only on the array and not dtype), and `is_sparse` which is also for array-like. 
On the other hand, `is_period` and `is_interval` then follow the other scheme where they are for the scalar objects, and the `_dtype` version for dtypes and array-likes. 
Anyway, mainly noting that it is already not fully consistent. But I agree it would be nice to follow the same scheme for all check functions.

I think this is a really bad idea to change. We have had this convention for a LONG time esp on the basic types. this is throwing out everything.
Throwing out?

________________________________
From: Jeff Reback <notifications@github.com>
Sent: Tuesday, July 24, 2018 5:05:31 PM
To: pandas-dev/pandas
Cc: Tom Augspurger; Mention
Subject: Re: [pandas-dev/pandas] REF/API: Stricter extension checking. (#22031)


I think this is a really bad idea to change. We have had this convention for a LONG time esp on the basic types. this is throwing out everything.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/22031#issuecomment-407567596>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIlZxP53Zujbodat9MFHLx1yidcOqks5uJ5orgaJpZM4VblLO>.

Are you -1 on this @jreback? If so you'll need an alternative fix to #22021, as I'm not sure how to avoid that without passing data to np.dtype.
as I said I am -1 on this. This changes too much. This changes the meaning of the is_* for a single case, which is contrary to all of the existing code. I am not sure what the problem is, but fixing this way is not it.
That's unfortunate.
Sorry, I closed this in frustration with you @jreback. But I think it's a good change for the project, so I'll continue to state my case for it.

Comments like

> I am not sure what the problem is

and

> This changes the meaning of the is_* for a single case, which is contrary to all of the existing code.

are frustrating, since they've already been addressed. I explain the original problem in the issue (https://github.com/pandas-dev/pandas/issues/22021#issuecomment-407157789) and here (https://github.com/pandas-dev/pandas/pull/22031#issuecomment-407414822).

Joris explains in https://github.com/pandas-dev/pandas/pull/22031#issuecomment-407415472, this follows the precedent laid out by categorical and datetimetz.
> > Wouldn't it be more logical to call this is_extension_array instead of is_extension_array_dtype? (I suppose the reason is backwards compatibility for at least one of the two use cases?)

> Yes. These weren't in the public namespace earlier, so we don't have to worry about deprecating.

But since they are not yet in the public namespace, we can choose whichever best name we can think of? Or maybe I didn't understand your answer :-) 

Anyway, I think we need to think a bit more on the naming scheme. 
`is_extension_dtype` (for the dtype checker) sounds good. But `is_extension_array_dtype` for array-like checking sounds a bit strange to me (although in the end you check the `dtype` of the array ...). `is_extension` might be OK here, but for the other dtypes will conflict with the scalar checkers. And `is_extension_array` is maybe too specific in "array" as it checks for any array-like that has a dtype (series, index, extensionarray, ..)? Maybe "arraylike" (but doesn't read that nice)?




> But since they are not yet in the public namespace, we can choose whichever best name we can think of? Or maybe I didn't understand your answer :-)

Sorry for the confusion. I think you understood my answer, but I didn't push the change :) I think `is_extension_array` for checking whether an array-like is an extension array is good.
this is exactly what pandas_dtype does
Yes, but `pandas_dtype` also potentially passes data to `np.dtype` in https://github.com/pandas-dev/pandas/blob/2c7c79779f1ef353d02446504d3b5f5c5cc68759/pandas/core/dtypes/common.py#L2013, which we need to avoid.
> but pandas_dtype also potentially passes data to np.dtype in

Since you now splitted it in two functions, in the new `is_extension_dtype`, you can only use `pandas_dtype` when the input is a string, and then you should never run into those problems?
Refactored.

1. No longer splitting `is_extension_dtype` and `is_extension_array_dtype`.
2. `is_extension_array_dtype` avoids calling `pandas_dtype`. There's no reason to go through all the stuff that `pandas_dtype` does, including `np.dtype(object)`, just to test if it's an extension array or dtype
3. DatetimeTZDtype and PeriodDtype were previously registered as extension dtypes. But this mixed dtypes that satisfy the interface and our own internal ones. For now, we'll need two registries, but this should be reduced to one by the time 0.24.0 comes out.
@TomAugspurger looks a lot nicer! 
I don't think so. pandas will still have the issue of

```python
In [28]: subarr = [('f1', 'f2')]

In [29]: pd.api.types.pandas_dtype(subarr)
Out[29]: dtype([('f1', '<f2')])
```

this is just a workaround that avoids the problem by not calling `pandas_dtype` from within `is_extension_array_dtype`.
can you rebase
thanks!!!