REF: Fix maybe_promote
State: closed

- [x] closes #23833
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

This PR is the culmination of ongoing work since the start of November, and is therefore a bit on the bigger side, with several notes to make.

Things started out with me wanting to unify `.update` for Series/DF (#22358), resp. aiming towards a beefed-up `update`/`combine_first`/`coalesce` (#22812). While tackling the former (#23192), I encountered some problems with `df.update` upcasting stuff unnecessarily (#23606), and while trying to fix it, I ran into problems with `maybe_upcast_putmask` (#23823), which were directly caused by the utterly broken (and completely untested) `maybe_promote` (#23833).

I started with writing some tests (#23982), which turned out to be not so trivial, because there's a lot of complexity, and the correct behaviour wasn't alwasy immediate (also encountered some fun numpy bugs in the process: e.g. numpy/numpy#12525, numpy/numpy#12550)

I set out to write out a PR to fix those tests then, with the obvious goal of getting the test suite to pass - already that required a full rewrite of the method. I cracked my own tests after a while, but the test suite eluded me. As it turns out, `maybe_promote` mixes two very different behaviours - scalar values get cast to the new dtype, whereas arrays return their missing value marker. I tried kludging around this for a while, and decided it wasn't possible without creating a franken-solution.

The next step was to *separate*  these two different behaviours into different functions, `maybe_promote_with_scalar` and `maybe_promote_with_array`, where `maybe_promote` is then just a thin wrapper that switches between the two. Actually also `maybe_promote_with_scalar` is just a fairly thin wrapper around `maybe_promote_with_array`, so that the actual many-cased promotion logic does not have to be implemented twice.

Often, the call-sites in the code just need the one or the other, and this could later be broken up correspondingly.

I updated the tests in #23982 (taking care to fully capture all the xfails there) and based this PR on that. This should give already an overview of what changed. In many cases, the current behaviour is broken, but I did make a few design decisions worth noting:
* `maybe_promote_with_array` consistently returns the missing value marker for the updated dtype. Since integer dtypes (plus bools and bytes) cannot hold `np.nan`, these cases now return `None`.
* all promotion logic is as conservative as possible, also within subtypes. For arrays, promotion always goes by value, and never by dtype. That means that, for example:
```
    >>> maybe_promote(np.dtype('uint8'), fill_value=np.iinfo('uint8').max + 1)
    (dtype('uint16'), 256)
    >>> maybe_promote(np.dtype('uint8'), fill_value=np.array([-1], dtype='int64'))
    (dtype('int16'), None)
```
* all promotion logic is as type-safe as possible, which means that [x] only stays [x] if the `fill_value` is of type [x] as well, where x is one of (datetime, timedelta, bool, bytes). Datetimetz must additionally match the timezone.
* all scalar `fill_values` now truly get cast to the updated dtype (before there were lots of ambiguities around int/float/complex/datetime/timedelta subtypes)
* I have changed the behavior that strings get interpreted for datetimes/timedeltas. Since this is an untested private method, and the test suite still passes just fine, I think this is actually a good thing, because it's too much in one method. String to datetime/timedelta *should* need an explicit cast, IMO.
```
    >>> # master
    >>> maybe_promote(np.dtype('datetime64[ns]'), '2018-01-01')
    (dtype('<M8[ns]'), 1514764800000000000)
    >>> # PR
    >>> maybe_promote(np.dtype('datetime64[ns]'), '2018-01-01')
    (dtype('O'), '2018-01-01')
    >>> # master
    >>> maybe_promote(np.dtype('timedelta64[ns]'), '1 day')
    (dtype('<m8[ns]'), 86400000000000)
    >>> # PR
    >>> maybe_promote(np.dtype('timedelta64[ns]'), '1 day')
    (dtype('O'), '1 day')
```
* `iNaT` is considered a missing value from the POV of `maybe_promote_with_array` in all situations. This takes one single integer out of the usable `int64`-range, but I think this is much cleaner.

There's still a few issues with `lib.infer_dtype` (e.g. #23554, of which I already fixed the complex case #25382), most notably that it cannot infer `datetime64tz` yet. Actually, through this PR, I'm learning how broken that method is *as well*, but fixing that will have to wait for some other time. Among other things, it currently faceplants for `PeriodArray` / `IntervalArray` (#23553). I haven't added tests for these types here, but ~9000 tests is already better than nothing, I hope. ;-)

Another point that could/should be considered is how EAs should deal with this (#24246).

Labels:
name: Bug
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff

Comments:
@h-vetinari pls make it bite-sized and piecemeal.These giant PR's very likely won't be merged as they take too much review time.
> @jreback: @h-vetinari pls make it bite-sized and piecemeal.These giant PR's very likely won't be merged as they take too much review time.

I split off the tests into #23982 already. This PR is just refactoring the method (~200LoC).

> just a brief glance shows this as way too complicated. This _must_ be split up function wise. So as I said on a prior PR this would need to be a separate module with supporting functions.

I'm not sure you were addressing me with that (or on which PR) - don't know which modularisation you mean...? In any case, I already made an attempt at modularising things, by splitting the scalar and array case into separate methods.

The method itself is not very complicated, it just has lots of branches (in steps 2 & 4 below) to deal with all the possible inputs:
1. determine scalar/array case
1. check if array is empty or all-na
1. infer dtype
1. handle promotion logic
1. (scalar case only) handle casting of `fill_value`
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25425?src=pr&el=h1) Report
> Merging [#25425](https://codecov.io/gh/pandas-dev/pandas/pull/25425?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/3855a27be4f04d15e7ba7aee12f0220c93148d3d?src=pr&el=desc) will **decrease** coverage by `50.04%`.
> The diff coverage is `47.65%`.

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

```diff
@@             Coverage Diff             @@
##           master   #25425       +/-   ##
===========================================
- Coverage   91.73%   41.69%   -50.05%     
===========================================
  Files         173      173               
  Lines       52856    52932       +76     
===========================================
- Hits        48490    22072    -26418     
- Misses       4366    30860    +26494
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.69% <47.65%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25425?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `48.22% <47.65%> (-39.95%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| ... and [131 more](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25425?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/25425?src=pr&el=footer). Last update [3855a27...6792a54](https://codecov.io/gh/pandas-dev/pandas/pull/25425?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/25425?src=pr&el=h1) Report
> Merging [#25425](https://codecov.io/gh/pandas-dev/pandas/pull/25425?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/e9f9ca1f18f22215bb32cfd182f69997792b50e4?src=pr&el=desc) will **increase** coverage by `0.13%`.
> The diff coverage is `93.7%`.

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

```diff
@@            Coverage Diff             @@
##           master   #25425      +/-   ##
==========================================
+ Coverage   91.85%   91.99%   +0.13%     
==========================================
  Files         180      180              
  Lines       50765    50850      +85     
==========================================
+ Hits        46631    46777     +146     
+ Misses       4134     4073      -61
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.63% <93.7%> (+0.14%)` | :arrow_up: |
| #single | `41.83% <47.24%> (-0.12%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25425?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `90.88% <93.7%> (-0.18%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `88.88% <0%> (-11.12%)` | :arrow_down: |
| [pandas/core/arrays/integer.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2ludGVnZXIucHk=) | `96.3% <0%> (-1.32%)` | :arrow_down: |
| [pandas/core/internals/construction.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2NvbnN0cnVjdGlvbi5weQ==) | `95.95% <0%> (-0.8%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `94.38% <0%> (-0.72%)` | :arrow_down: |
| [pandas/core/dtypes/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbmNhdC5weQ==) | `96.58% <0%> (-0.46%)` | :arrow_down: |
| [pandas/core/internals/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2NvbmNhdC5weQ==) | `96.48% <0%> (-0.37%)` | :arrow_down: |
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `94.19% <0%> (-0.31%)` | :arrow_down: |
| [pandas/core/internals/managers.py](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL21hbmFnZXJzLnB5) | `96% <0%> (-0.22%)` | :arrow_down: |
| ... and [30 more](https://codecov.io/gh/pandas-dev/pandas/pull/25425/diff?src=pr&el=tree-more) | |

------

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

@TomAugspurger @jbrockmendel 
Care to take a look here or in #23982 please?
@h-vetinari I'll take a look at this.  My schedule is unusually hectic between now and Tuesday, so it might take a few days.
> @jbrockmendel: @h-vetinari I'll take a look at this.

Thanks!
@h-vetinari does this still need review or is the game-plan to do something else first?
> @jbrockmendel: @h-vetinari does this still need review or is the game-plan to do something else first?

Thanks for asking. This is ready as far as I'm concerned, but it *is* quite big. As such, I've split off the tests to #23982. However, since `maybe_promote` is untested and quite broken, this ends up adding too many xfails for the liking of @jreback, so I've started a pared-down precursor to the tests with #25637.

Both of the testing PRs could use a review from the POV of whether you like what's being tested. :)
closing as stale
@jreback 
Not stale, see [comment](https://github.com/pandas-dev/pandas/pull/25637#issuecomment-491683559). Please reopen.
@simonjayhawkins 
Thanks for reopening.
> @jreback: haven’t looked recently but this is looking very complicated at a glance

Since it's been a while, let me recall from further up the thread: `maybe_promote` is quite broken (see all the xfails I'm removing, resp. reinstating the tests that had too many xfails in #23982).

This PR completely refactors the existing `maybe_promote` method, and splits the existing (very different) behaviour for the scalar and array case (see OP).

The new method itself is not very complicated, it just has lots of branches (in steps 2 & 4 below) to deal with all the possible inputs:
1. determine scalar/array case
1. check if array is empty or all-na
1. infer dtype
1. handle promotion logic
1. (scalar case only) handle casting of `fill_value`
@jreback @jbrockmendel @TomAugspurger @simonjayhawkins 
Any chance of having another look at this? Fixing `maybe_promote` is what's keeping me from finishing #23192, resp. fixing #22358 #22812 #23606 #23833.

PS. I'd be happy to reinstate #23982, if only just to illustrate just how many things about `maybe_promote` are currently inconsistent/broken.
There's a lot here and most of it looks good, but as mentioned above, it is tough to review all at once.  It seems like there are a bunch of cases you are fixing here; can you fix them one at a time?  Then do the refactor at the end?
> I did that as far as I could by splitting it into #23982 and #25637. The method itself is a complete rewrite, I literally chucked the old thing out completely and started from scratch. IMO it would be a frankenstein-job to kind of graft them together so that the diff magically gets smaller.

I get that, but this is a case where Frankensteining is our best shot at maintaining forward momentum.
@jbrockmendel 
Fair enough, I understand your POV. I'll be away for two weeks, but I'll have a look at rewriting the method less extensively.
can you merge master. sorry we have so many PRs, will take a look soon.
> @jreback: can you merge master. sorry we have so many PRs, will take a look soon.

No worries, this one is a biggie. But the benefits would be quite nice (e.g. lots of methods would benefit from more correct upcasting. This is what's held me up on `.update`, `combine_first`, etc.)
@h-vetinari I'd like to get some positive momentum back here.  Let's try to identify parts of this we can split off.  How about the tests?  Can we enable (and xfail) some of the tests that this PR changes?
@h-vetinari it looks like maybe_promote is only used in 8-10 places outside of the tests.  is the ndarray fill_value case even needed?  I find that makes the function much harder to reason about.
> @h-vetinari it looks like maybe_promote is only used in 8-10 places outside of the tests. is the ndarray fill_value case even needed? I find that makes the function much harder to reason about.

The low number of call-sites is what made me think that this can be fixed at all. I'm quite sure that the array-case was necessary, otherwise all those gymnastics could have been avoided. Should be simple enough to test - I'll just take out the array-branch from `maybe_promote` and see if the CI passes.
@jbrockmendel 
So, at first glance a failing CI would show the necessity of the array-case, but on second glance, all the failures come from #25431 resp. #23823.

Taking a step back, in #23823 I said
> In the context of #23192 (and #23604 / #23606), I want to use `pandas.core.dtypes.cast.maybe_upcast_putmask`,  because it solves exactly the problem I need it to solve.
Unfortunately, it does not work as advertised (and I already found the culprit).
The docstring says: 
```
def maybe_upcast_putmask(result, mask, other):
    """
    A safe version of putmask that potentially upcasts the result
    [...]
```

The culprit at the time was the array-codepath in `maybe_promote`, but with the last commit above, it seems that the code-base does not rely anymore on the fact that `maybe_promote` can consume arrays. As such, it would be possible to change the implementation of `maybe_upcast_putmask` to use something else (e.g. the array-code I have already, or something else entirely) or just temporarily skip/xfail the tests from #25431, and then have a much simpler `maybe_promote` replacement that only needs to handle the scalar case.

The downside to that is that it would be harder to keep the various places where promotion logic is defined in sync. Ultimately, I think there's an even larger clean-up necessary, involving `maybe_promote`, `maybe_upcast_putmask`, `lib.infer_dtype`, `maybe_convert_objects`, etc. which - IMO/AFAICT - should all share similar promotion logic based for example what already exists with the `Seen`-objects used e.g. in https://github.com/pandas-dev/pandas/blob/74cba561ece511e24abb5145225bf98a929ca6c9/pandas/_libs/lib.pyx#L1952

Don't know if or when I ever get around to that, my more immediate goals had been #23192 and #22812 (or rather, not so immediate anymore after over a year, haha ;-)).
Thanks for tracking down the history behind the ndarray support; I'll read up on that.

You've seen #28561 and #28564; let's try to find other parts of this that you can break off into similarly sized/scoped pieaces.  e.g. carving out something like maybe_promote_scalar.  Anything else come to mind?

Based on a local branch, taking the ndarray cases out of the existing maybe_promote tests simplifies them a _ton_.  Whether to re-implement the ndarray cases separately or just get rid of them depends on whether we can drop the ndarray case completely.

Many of the failing cases can be fixed by changing

```
fill_value = Timedelta(fill_value).value
```

to 

```
try:
    fv = Timedelta(fill_value)
except (TypeError, ValueError):
    dtype = np.object_
else:
    fill_value = fv.value
```

Same for the Timestamp case.

That said, I think that instead of `.value` these should be `to_timedelta64()` and `to_datetime64()`, respectively (except for the NaT case, which is another hassle).  Try to get away from using iNaT, which is ambiguous.  This change should be relatively late in the process.

Any other ideas?  I can keep pushing at this if you're not interested, but I'm hoping you'll keep going just in smaller bits.
> Based on a local branch, taking the ndarray cases out of the existing maybe_promote tests simplifies them a ton. Whether to re-implement the ndarray cases separately or just get rid of them depends on whether we can drop the ndarray case completely.

Yeah, this is what I meant above. If we keep everything about `maybe_promote` in the scalar case, then both the code and the tests could be substantially reduced, but would need refactoring. I'm thinking this should be an entirely separate PR, that might serve as a *different* goal to aim for (and to cut individual chunks off).

> That said, I think that instead of `.value` these should be `to_timedelta64()` and `to_datetime64()`, respectively (except for the NaT case, which is another hassle). Try to get away from using iNaT, which is ambiguous. This change should be relatively late in the process.

I tried to stay as close as possible to existing behaviour, but since we're (potentially) rewriting the method, it's completely fair to question the tests I've written. One choice I made is that I think strings should not be cast to TD/DT automatically. I've tried to comment such decisions in the implementation resp. the test module as well.

iNaT is used quite ubiquitously, so not sure how easy it is to get rid of it, I just considered it in the same category as the other NA-values. 
@h-vetinari can you rebase?  Any plans to do small-pieces PRs for this in the near future?  If not, I'm going to keep trying to chip away at this.
Sorry for the delay, will try to get to merging soon (should be latest on Sunday)
@h-vetinari are you planning to run with this?  Its fine if not, but I think you have a better idea of whats needed here than i do
@jbrockmendel 
The main question I had (see [this comment](https://github.com/pandas-dev/pandas/pull/23982#issuecomment-541348431)) was whether it's desired to support the array-case should in `maybe_promote` at all (since that seemed to be up in the air).

I overlooked your response there - sorry. I'm a bit swamped at the moment, but I'll try to carve out a PR from this one that adds `maybe_promote_ndarray`. Hopefully should have time on the weekend.

> @jbrockmendel: Do you have fixes in mind for the remaining xfailed cases? bite-sized PRs for those would be welcome.

I'll have to have a look at how the current `maybe_promote` works (I haven't kept up with all your PRs). The array case will probably need to be handled separately anyway. One thing I wanted to avoid initially is to duplicate the promotion logic in two places (since looping over the scalar `maybe_promote` for the array-case is not an option performance-wise). But I think I'll have to start like that rather than replacing the whole method in one go.

> @jbrockmendel: AFAICT the ndarray use cases of maybe_promote are a) pathological object-dtype cases where an ndarray is being treated like a scalar, and b) future use cases you describe. Am I missing any ways a user could get there at the moment?

Right now, the only case in the testing suite I'm aware of are the tests for #23823. Not sure if some other code is using the array-path...
@h-vetinari can you rebase