Provide ExtensionDtype.construct_from_string by default
State: closed

- [ ] closes #xxxx
- [X] tests added / passed
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

I think it makes sense to provide a standard `construct_from_string` by default, instead of forcing subclasses of `ExtensionDtype` to implement it.

This way we can define a simple dtype with:

```python
class SimpleDtype(pandas.core.dtypes.dtypes.ExtensionDtype):
    name = 'simple'

    @property
    def type(self):
        return object
```

instead of:
```python
class SimpleDtype(pandas.core.dtypes.dtypes.ExtensionDtype):
    name = 'simple'

    @property
    def type(self):
        return object

    @classmethod
    def construct_from_string(cls, string):
        if string != cls.name:
            raise TypeError("Cannot construct a '{}' from '{}'".format(
                cls.__name__, string))
        return cls()
```

CC: @TomAugspurger 

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

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=h1) Report
> Merging [#26562](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/a91da0c94e541217865cdf52b9f6ea694f0493d3?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `0%`.

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

```diff
@@            Coverage Diff             @@
##           master   #26562      +/-   ##
==========================================
- Coverage   91.77%   91.76%   -0.02%     
==========================================
  Files         174      174              
  Lines       50649    50652       +3     
==========================================
- Hits        46483    46479       -4     
- Misses       4166     4173       +7
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.29% <0%> (-0.01%)` | :arrow_down: |
| #single | `41.69% <0%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Jhc2UucHk=) | `94.44% <0%> (-5.56%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26562?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/26562?src=pr&el=footer). Last update [a91da0c...20ec4e4](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=h1) Report
> Merging [#26562](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/e0c41f79104c5bc61952c9a14f1883cd5bda53f7?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/26562/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #26562      +/-   ##
==========================================
- Coverage   91.87%   91.87%   -0.01%     
==========================================
  Files         174      174              
  Lines       50663    50657       -6     
==========================================
- Hits        46548    46539       -9     
- Misses       4115     4118       +3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.4% <100%> (ø)` | :arrow_up: |
| #single | `41.93% <100%> (+0.08%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/integer.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2ludGVnZXIucHk=) | `96.3% <ø> (-0.05%)` | :arrow_down: |
| [pandas/core/dtypes/dtypes.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2R0eXBlcy5weQ==) | `97.54% <ø> (+0.2%)` | :arrow_up: |
| [pandas/core/dtypes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Jhc2UucHk=) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26562/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26562?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/26562?src=pr&el=footer). Last update [e0c41f7...2897ad2](https://codecov.io/gh/pandas-dev/pandas/pull/26562?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Seems reasonable. I'm not sure what the motivation was for not doing that in the first place. Possibly that we internally call `construct_from_string()` inside `try / except TypeError` blocks in a few places, which would potentially mask the issue? But I think our tests should be adequate.
Hello @datapythonista! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:











There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-06-05 15:37:26 UTC
Thanks for the feedback. I fixed the docstrings, improving the example to show how to overwrite the method, and remove the method of the category dtype, since now was the same as the parent class.

I think in a follow up PR we could make a generic `construct_from_string` method that extracts the arguments from a string like `datetime64[ns, UTC]` and returns an instance of the class passing the list of everything in the squared brackets as `*args`. This way we shouldn't have to overwrite `construct_from_string` for any dtype. I guess I'm not missing anything.
@jorisvandenbossche @jreback I think the tests are in the location you meant now. Let me know if you have any further comment.
Thanks! That's what I was thinking about. You are happy with that as well?
 
I think there are still some custom implementation of `construct_from_string` (that are now identical as the parent one) in the test arrays that can be removed (in decimal, json, arrow/bool)

I think the tests now are testing something slightly different as before. They're testing that new extension dtypes satisfy the implementation of the base class. Before I was testing the base class itself. I think both are reasonable, I'm happy this way too, but I think it also made sense what I implemented before.

I deleted the two `construct_from_string` methods that were repeating the new base class behavior, there are no more to delete.

Thanks for the review, let me know if there is anything else that should be changed, I think it should be ready now.
thanks @datapythonista 
This apparently caused some performance issues: http://pandas.pydata.org/speed/pandas/index.html#sparse.SparseDataFrameConstructor.time_from_scipy?commits=5a724b5cd796a6ede3cb95b8687eaf561e9d57b2

Will open a proper issue later. 
It seems a bit strange that this PR is the cause of that regression, as SparseDtype has its own implementation of `construct_from_dtype`.
Quick profiling of the specific benchmark points out the culprit: in the dtype checking code if doing a try/except it formats the full series in the error message.
Ah, thanks for digging into it!

It's unfortunate that such a minor change can have such broad consequences on performance. I assume this is from something like `pandas_dtype(thing)`, where `thing` is an array-like rather than a dtype? If we were more careful about not passing data to `pandas_dtype`, we would have been OK?
We have had similar performance regressions before. In general we need to be careful with what we print in error messages, but this is of course easy to overlook, as we did in this case. But I would say: that's what we have benchmarks for, and they worked nicely to catch it here.
I did https://github.com/pandas-dev/pandas/pull/26776 with a possible fix. 

I also don't think it is that an important fix, as there are some strange things going on in `SparseDataFrame._init_spmatrix`, where they pass a dict of {colname: SparseSeries} to `Index.difference` method (while it should pass something like dict.values() I think), and so it is the conversion of this dict to an Index that is taking a lot of time (in there it is checking if this object is categorical, leading to the error message, as dicts are not special cased there, and leading to printing a dict of 1000 serieses). 
Given that this is all deprecated, I am not going to put effort in cleaning that up.