ExtensionDtype.construct_array_type is not optional

[jorisvandenbossche]

Currently, the ExtensionDtype docstring says:

Optionally one can override construct_array_type for construction
with the name of this dtype via the Registry. See
:meth:`pandas.api.extensions.register_extension_dtype`.

* construct_array_type

However, it seems that in practice this is used for more than just supporting string dtypes. Eg I also need to implement it to have groupby working.

Eg cyberpandas does not yet implement it, and there df.groupby('ipaddresses').size() fails with a NotImplementedError.

I assume we are fine with requiring this (assuming a 1-1 mapping of dtype and array), and a doc update is enough?

[jreback]

yeah this should probably be a required element of an EA Dtype

[TomAugspurger]

Yeah, OK to require it.

…

On Mon, Jan 21, 2019 at 8:19 AM Joris Van den Bossche < ***@***.***> wrote: Currently, the ExtensionDtype docstring says: Optionally one can override construct_array_type for construction with the name of this dtype via the Registry. See :meth:`pandas.api.extensions.register_extension_dtype`. * construct_array_type However, it seems that in practice this is used for more than just supporting string dtypes. Eg I also need to implement it to have groupby working. Eg cyberpandas does not yet implement it, and there df.groupby('ipaddresses').size() fails with a NotImplementedError. I assume we are fine with requiring this (assuming a 1-1 mapping of dtype and array), and a doc update is enough? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#24860>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIhOz-O2u_Dzc1kl1Y3PoA_tpxEdsks5vFcxngaJpZM4aK6pH> .

[jbrockmendel]

I see the usefulness here, but there is a non-trivial downside that this prevents core.dtypes from being self-contained (w/r/t the rest of core).

[jbrockmendel]

I'm now fully on board with this. Is it just a matter of updating the docstring?

[jorisvandenbossche]

And I suppose changing the implementation from NotImplementedError to AbstractMethodError ?