API: Series.str-accessor infers dtype (and Index.str does not raise on all-NA)
State: closed

closes #23163 
closes #23011 
closes #23551
 ~#23555 / #23556~
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Several times while working around `str.cat` (most recently #22725) @jreback and @WillAyd mentioned that the `.str` accessor should *not* work for (e.g.) bytes data. So far, the constructor of the `StringMethods` object didn't infer for `Series` -- this PR adds that.

~~What needs disussion is the following: I've commented out two tests about an encode-decode-roundtrip that cannot work when forbidding bytes data. Should such an encode-decode-cycle explicitly part of the desired functionality for `.str` (currently there are `.str.encode`  and `.str.decode` methods), or not? I guess that even if not, this would need a deprecation cycle.~~

Alternatively, it'd be possible to allow construction of the `StringMethods` object but raise on `inferred_dtype='bytes'` for all but the `str.decode` method.

Finally, it would be possible to partially close #13877 by also excluding `'mixed-integer'` from the allowed inferred types, ~~but for floats, the inference-code would need to be able to distinguish between actual floats and missing values (currently, both of those return `inferred_type='mixed'`)~~


Labels:
name: Strings
description: String extension data type and string data

Comments:
Hello @h-vetinari! 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-05-30 15:17:50 UTC
Hm, turns out the `.str`-on-bytes is used in several more places. Reverting that part for now.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23167?src=pr&el=h1) Report
> Merging [#23167](https://codecov.io/gh/pandas-dev/pandas/pull/23167?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/a60d1bd45a99519fad5024068db956e0aa1cc6a1?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/23167/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23167?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #23167      +/-   ##
==========================================
- Coverage   91.84%   91.84%   -0.01%     
==========================================
  Files         174      174              
  Lines       50643    50682      +39     
==========================================
+ Hits        46515    46550      +35     
- Misses       4128     4132       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.38% <100%> (ø)` | :arrow_up: |
| #single | `41.74% <95.52%> (-0.03%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23167?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/23167/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.92% <100%> (+0.05%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/23167/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/23167/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/23167?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/23167?src=pr&el=footer). Last update [a60d1bd...f62e344](https://codecov.io/gh/pandas-dev/pandas/pull/23167?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jreback 
> So what i'll suggest is that we do an infer pass at the very beginning, rejecting the obvious things, then pass that inferred string around so that the methods can decide what to do, and have several canned responsed, eg.. a standard warning, or an error (for the bytes things).

I implemented just that for the bytes case now. PTAL

Side note: I needed to use #23422 already to be able to use the type inference.
@jreback 
Since all these things are interrelated, the only way I could think of how to split this PR, is adding the (initially mostly xfailed) tests of this PR in #23582, and then all the code fixes can follow independently. 
@jreback 
> so this PR is too much going on. pls break apart with a sepearate PR

Agreed, which is why I split off the tests (which are the main source of changes, and responsible for checking them) in #23582. Let's do that one first, and then I can easily split up this PR.
@h-vetinari https://github.com/pandas-dev/pandas/pull/23167#issuecomment-437687958

great thanks. The reason I am no insistent on not having giant PRs is that the iteration cycle bascially makes them impossible. Having 1 thing in a PR allows merging in a resaonbale amount of time.Sure you can close multiple issues, but having diffs from moved files or massive changes makes checking for changes so much more important. Smaller single focused PR's are much more palatable.
Recording a conversation with @toobaz from #23670 for later reference:

@toobaz 
> [...] only method [...] I can think of which gives a different result when called on a flat `Index` and when called on an equivalent 1-level `MultiIndex`

@h-vetinari 
> FWIW, `.str.cat` works for `Index`, but not a 1-level `MultiIndex`, which I guess qualifies as a different result. ;-)
There's probably many more broken methods for 1-level `MultiIndex` in the `.str` accessor (the `.str`-constructor passes for 1-level `MultiIndex`, but the methods behind it are completely untested for this case). My current idea/plan is to disable `.str` for `MultiIndex` in #23167 (which is in the process of being split into several PRs). [...]

@toobaz 
> Sorry, naive question, but what is the problem with just running `.str` on the result of `self.get_level_values(0)`?

@h-vetinari 
> Fair point, will most likely use that. Just didn't bother investigating further when I saw those were mostly broken. Only question then would be the following: Currently, `Index.str.<method>` always returns an `Index`. It'd be easy to return the same for 1-level `MultiIndex`, but would that change (from MI->`Index`) be confusing/surprising from a user-POV? [...]

The discussion there is currently continuing whether 1-level `MultiIndex` should be allowed at all, a proposal which would obviate the need for any further action here.
@jreback 
I reduced this as much as possible to make it an easier continuation from #23582. I'll open up PRs for the rest soon.

This reduces many of the xfails, but also uncovered some new ones that were hiding behind *other* errors.

PTAL whenever you can.
@jreback 
I split off #24005 and #24045, and have two more that only work as follow-ups. PTAL :)
@jreback 
I'll fix the conflicts as soon as #24005 is in as well. Just checking that the rest of the PR is solid enough. Especially as a follow-up for a `.bytes` accessor seems necessary.
i haven't looked at this in detail yet; let's get the others in first.
Merged in #24005 in anticipation that it will be merged soon (plus merged in #24045 as well).
@jreback 
Now that #24005 and #24045 are in, this should be ready to review
@jreback
Do you still want this for v0.24? I know I haven't the whatsnew-section yet (been busy with the `maybe_promote` PR), but I'm actually a bit hesitant to push this in. As you mentioned in passing above, this change would reasonably require adding a `.bytes`-accessor, and that sounds like a bigger change.

The downside would be that this change then has to wait until 1.0, as it's breaking.
@jreback @jorisvandenbossche @TomAugspurger 
Should I still go forward with this? There's a bunch of fixes predicated on this (partial list in OP), but it would most likely be necessary to replace the existing functionality for bytes with a `.bytes`-accessor, and that would obviously be breaking. Not sure if that's something that would even be considered now for 0.25?
I see that #25124 has added a breaking change (if even a relatively minor one) - so does this mean that breaking changes are back for 0.25? Or is it on a case-by-case basis? If the latter, should I still push forward with this?

@TomAugspurger @jreback @jorisvandenbossche 
> looks pretty good. pls make a sub-section in the what's new showing the minimium what a user would want to know about these changes. you can link to a section in text.rst if needed (you an probably just add a warning box at the top level saying that .str ops mainly work on actual strings, with a few exceptions).

Alright, finally got around to adding something about this in `text.rst` and a little section in the whatsnew.
@h-vetinari if you want to merge master and respond to my commments. generally this looks good, though some comments on the impl.
pls merge master
this looked ok, if @pandas-dev/pandas-core wants to finish this up.
@jreback 
Couldn't respond to your last ping at the time and then forgot. Merged master.
@h-vetinari a couple of comments, merge master and ping on green for merge.
@jreback 
This is green.

Note that I enabled `'bytes'` also for methods `.str.get` and `.str.slice`, as those are still useful in that case, and to reduce the potential impact of the breaking change.
Argh, just double-checked and saw that I had forgotten to save the whatsnew changes...
thanks @h-vetinari 

as you have seen, targeted PRs merge *much* much faster than giant refactors
> @jreback: as you have seen, targeted PRs merge much much faster than giant refactors

Fair, but that's not a good reason to completely avoid all refactors. Sometimes, painful(ly large) PRs are necessary.