Update type for PeriodDtype / DatetimeTZDtype / IntervalDtype

[TomAugspurger]

Split from PeriodArray.

Updating the type to be the more correct Period.

Also removed the essentially unused IntervalDtypeType. If this doesn't break anything, it seems like there are decent chunks of _get_dtype_type that aren't being hit, aside from test__get_dtype_type.

[pep8speaks]

Hello @TomAugspurger! Thanks for submitting the PR.

• There are no PEP8 issues in the file pandas/core/dtypes/common.py !

• There are no PEP8 issues in the file pandas/core/dtypes/dtypes.py !

• There are no PEP8 issues in the file pandas/tests/dtypes/test_common.py !

[codecov]

Codecov Report

Merging #22938 into master will decrease coverage by <.01%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master   #22938      +/-   ##
==========================================
- Coverage   92.19%   92.19%   -0.01%     
==========================================
  Files         169      169              
  Lines       50822    50820       -2     
==========================================
- Hits        46855    46852       -3     
- Misses       3967     3968       +1

Flag	Coverage Δ
#multiple	90.6% <93.75%> (-0.01%)	⬇️
#single	42.36% <68.75%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/dtypes/base.py	100% <ø> (ø)	⬆️
pandas/core/dtypes/common.py	95.03% <100%> (+0.01%)	⬆️
pandas/core/dtypes/dtypes.py	95.82% <87.5%> (-0.3%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9caf048...bccfc3f. Read the comment docs.

[jreback]

this is really changing the semantics

[TomAugspurger]

Which semantics?

[jreback]

all the other dtypes return the meta class
now you are returning the actual class (and not even the type)

this is not right

[jorisvandenbossche]

This is what we have been doing for all EAs.

Now in practice, I don't if we ever rely on the value, though?

[jreback]

I agree that am not sure this is actually used. But If you are chaning, try changing for all the dtypes? (or don't change). that's what I mean by semantics, we neeed to be consistent.

[TomAugspurger]

From the ExtensionArray docs

        """The scalar type for the array, e.g. ``int``

        It's expected ``ExtensionArray[item]`` returns an instance
        of ``ExtensionDtype.type`` for scalar ``item``.
        """

Period is the scalar type for arrays with PeriodDtype, in the sense that isinstance(periodarray[0], Period) is true. What's the issue?

now you are returning the actual class (and not even the type)

Classes are types (they can be used as the second argument to isinstance).

[TomAugspurger]

But If you are chaning, try changing for all the dtypes? (or don't change)

We discussed earlier change CategoricalDtype.type to be object, but IIRC that was rejected (I don't recall the specifics).

Happy to change DatetimeTZDtypeType (to Timestamp).

[jreback]

can you see if we have tests that hit any of this? (if not pls add). happy to change to either way, but should just change all.

[TomAugspurger]

tests/dtypes/test_common.py::test__get_dtype_type hits this. Otherwise, the only path to here seems to be _get_dtype_or_type.

[jorisvandenbossche]

And even then, for EA's it is actually never used in _get_dtype_or_type (I mean, there is no check function relying on that value, only for numpy dtypes)

[TomAugspurger]

Something that just occurred to me: We have types where "missing" is represented by a value that isn't an instance of the type. e.g. NaT, the missing value for Datetime and Period, isn't an instance of Timestamp or Period. Interval uses np.nan.

Should the interface define that a missing value for a dtype be an instance of the type? It's not clear to me that it should. Are people OK with saying that NA values are allowed to be other types?

[jorisvandenbossche]

Should the interface define that a missing value for a dtype be an instance of the type? It's not clear to me that it should. Are people OK with saying that NA values are allowed to be other types?

Well, currently I think almost none of our actual EAs have an na_value of their type, so I think that clearly answers the question?
I also don't see how it is possible to have this in general, as you don't always have control over the scalar type.

[jreback]

same here

[jreback]

this change still seems wrong

e.g.

In [1]: np.dtype('float64').type
Out[1]: numpy.float64

In [2]: type(np.dtype('float64').type)
Out[2]: type

is what we are emulating here. This is a wholesale change of this. I am not sure why you actually need to do this. Having a concrete type (e.g. a Period) rather a type(class), e.g. the type of the class is semantically correct here. I cannot believe this doesn't break anything.

[TomAugspurger]

Your code shows that this PR is doing the right thing.

In [3]: np.array([1, 2]).dtype.type
Out[3]: numpy.int64

In [4]: type(np.array([1, 2]).dtype.type)
Out[4]: type

In [6]: pd.core.arrays.period.PeriodArray([1, 2], 'D').dtype.type
Out[6]: pandas._libs.tslibs.period.Period

In [7]: type(pd.core.arrays.period.PeriodArray([1, 2], 'D').dtype.type)
Out[7]: type

[jreback]

why do you need to change at all? what exactly is breaking on master?

[TomAugspurger]

It's incorrect according to the definition of an ExtensionArray. I'm updating our internal arrays to implement the interface, and this is a standalone change that can be easily reviewed and approved since it's a simple, uncontroversial change.

…

On Tue, Oct 2, 2018 at 3:45 PM Jeff Reback ***@***.***> wrote: why do you need to change at all? what exactly is breaking on master? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22938 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIg4vey1pQWOdXD80oXL7EvuhgrI3ks5ug9CGgaJpZM4XDOmT> .

[jorisvandenbossche]

I cannot believe this doesn't break anything.

Because we actually never use this / rely on this. Or at least, only use dtype.type when .dtype is a numpy dtype.

We discussed before that the ExtensionDtype.type was more "pro forma" and not really doing anything (at the moment).

But if we keep it, I agree with Tom that we should at least have our internal EA dtypes to be consistent with the interface (which is what this PR is doing).

[jreback]

so i am not against changing for code cleanliness and maybe i am pounding sand here

but this makes EA dtypes inconsistent with numpy dtypes

now it probably doesn’t make any practical difference but maybe let’s change the text that they are (not a real numpy dtype)

they will now not even pretend to be a duck like numpy dtype at all

[TomAugspurger]

but this makes EA dtypes inconsistent with numpy dtypes

I think that's the misunderstanding. This is making it consistent with NumPy. The dtype.type should be the scalar type (int, float, Period, Timestamp).

…

On Tue, Oct 2, 2018 at 5:55 PM Jeff Reback ***@***.***> wrote: so i am not against changing for code cleanliness and maybe i am pounding sand here but this makes EA dtypes inconsistent with numpy dtypes now it probably doesn’t make any practical difference but maybe let’s change the text that they are (not a real numpy dtype) they will now not even pretend to be a duck like numpy dtype at all — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22938 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIkaIG11j51aOS6v92wjWP9pQahctks5ug-7wgaJpZM4XDOmT> .

[TomAugspurger]

but this makes EA dtypes inconsistent with numpy dtypes

@jreback does
#22938 (comment) make sense? On master we're inconsistent with numpy, since for these types .type isn't the scalar type of the array.

In code that's

In [2]: arr = np.array([1, 2])

In [3]: assert type(arr[0]) == arr.dtype.type

In [4]: parr = pd.PeriodIndex([2000, 2001], freq="D")

In [5]: assert type(parr[0]) == parr.dtype.type
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-5-2e0cefc2b29e> in <module>
----> 1 assert type(parr[0]) == parr.dtype.type

AssertionError:

In [6]: type(parr[0]), parr.dtype.type
Out[6]: (pandas._libs.tslibs.period.Period, pandas.core.dtypes.dtypes.PeriodDtypeType)

[TomAugspurger]

Only failure here is the S3 failure, which was hopefully resolved on master.

[jreback]

what was rationale for not making this object for Categorical here? otherwise this is the odd man out

[jorisvandenbossche]

I don't recall the previous discussion, but: categorical can basically be everything (so object would still be generic, that is true), but object is actually not a proper callable like the other types (you cannot do object(1), like you can do int(1), np.int64(1), or Period(..) ), so it doesn't follow the scheme.

In principle we could pass through the type of the categories, but the problem is that those can be None.

Since the type is actually never used (for the extension dtypes), I would also not bother too much about this one inconsistency.

[TomAugspurger]

Joris is correct.

The only additional comment is that having a .type of object doesn't really tell you anything useful. But something like Period, Interval, or Timestamp actually is meaningful.

[jreback]

so why don't we return None then? I think it is going to be very confusing that only Categorical as a DtypeType, but nothing else does. If you are getting rid of it, then remove it completely.

[jorisvandenbossche]

None has the same drawbacks of object (cannot be called, is overly generic, does not hold any useful information). The CategoricalDtypeType at least has in its name it is the type of the CategoricalDtype

[TomAugspurger]

Agreed with Joris.

[jreback]

ok, then