TST: add test coverage for maybe_promote
State: closed

- [x] first step for #23833 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] fixes a regression that was silently introduced in #23796 due to lack of tests

The errors I found for `core.dtypes.cast.maybe_promote` in #23833 were just scratching the surface. I spent about a week on this, because I had to figure out the right behaviour and how to test it simultaneously. I also spent quite some time in making the xfails as sharp as possible, so that they can almost verbatim be turned into counter-examples.

This surely needs a good review, especially for the datetimelike stuff, where I'm not 100% sure whether the interaction with float/integers should cast to object, but it would be my gut feeling.

In any case, this uncovered *lots* of errors, of which I've added some samples below the fold.

The larger points are the following (after which the tests have been modelled):
* `maybe_promote(dtype, fill_value)` fundamentally asks whether it's necessary to upcast `dtype` to accommodate a scalar `fill_value`.
  * In doing so, it should (IMO) be as conservative as possible, i.e. `int8 -> int16` instead of `int8 -> int64`. (currently broken as `int + int -> float`, always)
* It makes sense to use arrays for `fill_value`,  as a sort of vectorized operation for the scalar case (this is used like that in several places, e.g. `maybe_upcast_putmask`)
  * Continuing from the above point, `maybe_promote` should IMO not simply absorb the dtype of `fill_value` (otherwise, might as well call it `always_promote`...), but should be equally prudent whether actual upcasting is necessary.
  * this would mean inspecting the arrays with something like `infer_dtype` (at least in the case of object-typed `fill_value`)
* currently, the second component of the return value is quite useless and inconsistent. I assume it was originally meant to represent the missing value marker for the resulting dtype, but this is not the case for arrays, and doesn't make sense for `int -> int`, for example.
  * I would suggest to add a `return_na_value`-kwarg to determine whether a missing value marker should be returned at all (many places in the code don't care about the second output, see #23833).
* in #23796, @jreback asked me to remove some try/excepts that looked like they were left-overs from ancient numpy. The CI didn't fail on this *only* because the method was not tested sufficiently. As such I've fixed it directly.
* There's a fixture for `all_numpy_dtypes` that got introduced in #21899, but it's a lie - it doesn't cover (nearly) all numpy dtypes. It's currently only used in one test, and so I've adapted it here to be able to *actually* test all numpy dtypes. Nevertheless, I'll split off this part in a separate PR (xref coming).

Here's an incomplete extract of the many things that are going wrong currently:

<details>

```
>>> import numpy as np
>>> from pandas.core.dtypes.cast import maybe_promote
>>>
>>> # should be int16, not int32
>>> maybe_promote(np.dtype('int8'), np.iinfo('int8').max + 1)
(dtype('int32'), 128)
>>>
>>> # should be object, not raise
>>> maybe_promote(np.dtype(int), np.iinfo('uint64').max + 1)
Traceback (most recent call last):
[...]
OverflowError: Python int too large to convert to C long
>>>
>>> # should stay signed, not switch to unsigned
>>> maybe_promote(np.dtype('uint8'), np.iinfo('uint8').max + 1)
(dtype('int32'), 256)
>>>
>>> # should cast to int16, not int32
>>> maybe_promote(np.dtype('uint8'), np.iinfo('int8').min - 1)
(dtype('int32'), -129)
>>> 
>>> # should stay int
>>> maybe_promote(np.dtype('int64'), np.array([1]))
(<class 'numpy.float64'>, nan)
>>>
>>> # should upcast to object, not float
>>> maybe_promote(np.dtype('int64'), np.array([np.iinfo('int64').max + 1]))
(<class 'numpy.float64'>, nan)
>>>
>>> # should only upcast to float32
>>> maybe_promote(np.dtype('int8'), np.array([1], dtype='float32'))
(<class 'numpy.float64'>, nan)
>>>
>>> # should upcast to float64
>>> maybe_promote(np.dtype('float32'), np.finfo('float32').max * 1.1)
(dtype('float32'), 3.7431058130238175e+38)
>>>
>>> # should only upcast to complex64, not complex128
>>> maybe_promote(np.dtype('float32'), 1 + 1j)
(<class 'numpy.complex128'>, (1+1j))
>>>
>>> # should not upcast
>>> maybe_promote(np.dtype('bool'), np.array([True]))
(<class 'numpy.object_'>, nan)
>>>
>>> # should still return nan, not iNaT
>>> maybe_promote(np.dtype('bool'), np.array([1], dtype=np.dtype('datetime64[ns]')))
(<class 'numpy.object_'>, -9223372036854775808)
>>>
>>> # inconsistently transforms fill_value
>>> maybe_promote(np.dtype('datetime64[ns]'), True)
(<class 'numpy.object_'>, nan)
>>>
>>> # should upcast to object
>>> maybe_promote(np.dtype('datetime64[ns]'), np.array([True]))
(dtype('<M8[ns]'), -9223372036854775808)
>>>
>>> # should upcast to object
>>> maybe_promote(np.dtype('bytes'), np.array([True]))
(dtype('S'), nan)
>>>
>>> # inconsistently transforms fill_value
>>> maybe_promote(np.dtype('datetime64[ns]'), np.datetime64(1, 'ns'))
(dtype('<M8[ns]'), 1)
>>>
>>> # should (IMO) cast to object (cf below)
>>> maybe_promote(np.dtype('datetime64[ns]'), 1e10)
(dtype('<M8[ns]'), 10000000000)
>>>
>>> # inconsistently transforms fill_value
>>> maybe_promote(np.dtype('datetime64[ns]'), 1e20)
(<class 'numpy.object_'>, nan)
>>>
>>> # does not upcast correctly (but implicitly turns string to int)
>>> maybe_promote(np.dtype('timedelta64[ns]'), '1')
(dtype('<m8[ns]'), 1)
>>>
>>> # should upcast to object
>>> maybe_promote(np.dtype('int64'), np.timedelta64(1, 'ns'))
(dtype('<m8[ns]'), numpy.timedelta64(1,'ns'))
>>>
>>> # should upcast to object
>>> maybe_promote(np.dtype('float64'), np.timedelta64(1, 'ns'))
(dtype('float64'), numpy.timedelta64(1,'ns'))
>>>
>>> # should upcast to object
>>> maybe_promote(np.dtype('int64'), np.array([1], dtype=str))
(<class 'numpy.float64'>, nan)
>>>
>>> # should upcast to object
>>> maybe_promote(np.dtype(bytes), np.nan)
(dtype('S'), nan)
>>>
>>> # falsely mangles None into NaN
>>> maybe_promote(np.dtype(object), None)
(<class 'numpy.object_'>, nan)
```

</details>

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

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-10-11 13:29:07 UTC
> fixes a regression that was silently introduced in #23796 due to lack of tests

what was the regression?
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23982?src=pr&el=h1) Report
> Merging [#23982](https://codecov.io/gh/pandas-dev/pandas/pull/23982?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/558f754e1787625d8b18cd4960fa059d3533faae?src=pr&el=desc) will **decrease** coverage by `1.31%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #23982      +/-   ##
==========================================
- Coverage   93.06%   91.74%   -1.32%     
==========================================
  Files         192      173      -19     
  Lines       49507    52856    +3349     
==========================================
+ Hits        46075    48495    +2420     
- Misses       3432     4361     +929
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.31% <ø> (-1.52%)` | :arrow_down: |
| #single | `41.69% <ø> (-0.75%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23982?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-75%)` | :arrow_down: |
| [pandas/compat/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9fX2luaXRfXy5weQ==) | `58.1% <0%> (-36.77%)` | :arrow_down: |
| [pandas/plotting/\_misc.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19taXNjLnB5) | `38.68% <0%> (-26.18%)` | :arrow_down: |
| [pandas/io/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2NvbW1vbi5weQ==) | `72.86% <0%> (-21.25%)` | :arrow_down: |
| [pandas/io/gcs.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2djcy5weQ==) | `80% <0%> (-20%)` | :arrow_down: |
| [pandas/io/s3.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3MzLnB5) | `86.36% <0%> (-13.64%)` | :arrow_down: |
| [pandas/io/excel/\_util.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL191dGlsLnB5) | `78.82% <0%> (-11.18%)` | :arrow_down: |
| [pandas/core/computation/expr.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tcHV0YXRpb24vZXhwci5weQ==) | `88.17% <0%> (-9.35%)` | :arrow_down: |
| [pandas/core/groupby/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9iYXNlLnB5) | `91.83% <0%> (-8.17%)` | :arrow_down: |
| [pandas/plotting/\_core.py](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb3JlLnB5) | `83.58% <0%> (-6.05%)` | :arrow_down: |
| ... and [197 more](https://codecov.io/gh/pandas-dev/pandas/pull/23982/diff?src=pr&el=tree-more) | |

------

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

@jreback 
Merged in #23983 and green.
They share almost the same code path, and have the same tests. If I don't have a joint fixture, the two tests would be copied essentially verbatim.

Also, they're already together e.g. in the `any_numpy_dtype` fixture (and also similar to how signed and unsigned are together in `any_integer_dtype`).

But if having two copies each of the two tests (two for datetime, two for timedelta) is your preference - no problem.
@jreback 
Split things up into two fixtures and two x two tests now. All green too.
I realised that the current state of the PR was actually not that consistent, in that the statement of the docstring was not accurately reflected in what was being tested (that's because most of the current `fill_value` cases actually just return the value verbatim).

Since would not have been a very consistent state of affairs, I'm now aiming the tests exactly at the desired state of affairs, rather than having a few less xfails.

I also realised that datetime and timedelta should *not* be compatible with each other (i.e. always upcast to object), and adapted that as well.
@jreback 
Just realized we're in the last month of v.0.24 development. This PR blocks a couple issues (leading to #23192) that might be nice to have before release.
we’re are way over for 0.24

changes will be merged if they can be accommodated 
Milestone says until end of December: https://github.com/pandas-dev/pandas/milestone/55

What I meant is: Please take a look as soon as you can.
@h-vetinari i keep moving the milestone. What this means is I am not adding anything to the milestone until / until its exactly ready to merge.
> @h-vetinari i keep moving the milestone. What this means is I am not adding anything to the milestone until / until its exactly ready to merge.

Fair enough. In any case, if this PR is merged, the path to #23192 is pretty clear/quick.
Shifted the modules to a new subfolder as desired
@jreback 
Failure was isort, which had been previously ignored, but not anymore since I added the `/cast/` folder. Happy to isort the file, but wanted to keep the diff small.
The doc job had a 500 error - could someone restart it please? (@gfyoung ?)
@jreback 
After the recent CI woes are finally surmounted: PTAL. :)
@h-vetinari this has too many xfailed tests. I am not convinced we actualy need this many tests on this routine. if you want to purse this, ok, but  pls comment out 90% of them, this just causes the logs to be polluted. 
@jreback 
> @h-vetinari this has too many xfailed tests. I am not convinced we actualy need this many tests on this routine. if you want to purse this, ok, but pls comment out 90% of them, this just causes the logs to be polluted.

I'm working on a branch that is based on this to see how an implementation that passes these tests would look like. Can discuss further once I have that ready.
@h-vetinari can you merge master and I'll take a look.

I tracked a bug in DatetimeArray.fillna down to maybe_promote returning `iNaT` in a case when it should return `NaT`.  I'm hoping I can piggy-back on this in fixing it.
@jbrockmendel 

I spent almost all my dev-time in December on working on a follow-up to this that would actually fix (and not just xfail) these tests. This took me a couple of weeks, because there were/are several bugs (including some numpy ones), and it's a piece of code that gets called in many central places.

Once I had the promotion-tests passing (essentially after a complete rewrite of `maybe_promote`), I tried making the test suite pass, but ran into problems, because `maybe_promote` has some undocumented details that are necessary in some places. Particularly, the fill-value will get *interpreted* and transformed for scalar `fill_value`, which is fundamentally different from the use-case of an array-like `fill_value`, where one then returns the default missing value for the new dtype.

Instead of kludging my way forward, I decided I want to split out `maybe_promote_with_scalar` and `maybe_promote_with_array`, where `maybe_promote` can just switch depending on the case. I haven't managed to finish that yet, and will only be able to do so at the earliest on the 6th (even if I'm just aiming to push a WIP).

It's worth noting that I had to substantially enlarge `test_promote.py` from the state of this PR.

To be honest, it sounded like the change would be large enough that I didn't want to try to squeeze it into 0.24, but I'll try to make some headway on Sunday.
Thanks for the update.  Definitely don't rush yourself on my account.  (the immediate problem I was having has been fixed)
@h-vetinari so this is a nice idea, but there are couple of issues here:

1) too many xfails, this is just going to disrupt the builds and obscure anything else. you can mitigate this by putting the functions that *mostly* xfail in another file (test_maybe_promote_xfail) and just xfailing that entire file; when things are fixed you can move back to the main file
2) the functions needs to have ids in the parameterization for easy identification of tests

I updated this PR based on the work in #25425. The tests are the same in both PRs, but in #25425, they're easier to read without all the `xfail`-branches.
How about you split off the conftest updates & put up tests that have *very* few xfails, basically testing existing behavior.
> @jreback: How about you split off the conftest updates & put up tests that have _very_ few xfails, basically testing existing behavior.

I mean it's easy enough to remove tests from this PR, it's just that a lot of the current behaviour is broken, so why not reflect that? I would see it as a "companion PR" to #25425, where this PR one adds the tests, and #25425 refactors the method. If you don't want the xfails, they could just be merged in close succession.

In any case, I have a branch prepared that disables or removes some of these tests. **How does 10% xfail rate sound?**
@jreback 
Let me ask again (re: reduced scope for adding tests for `maybe_promote`):
> How does 10% xfail rate sound?
@jreback 
Please take the time to answer this simple yes/no question: Would a 10% xfail rate (relative to added tests) be OK for you?

> @jreback Let me ask again (re: reduced scope for adding tests for maybe_promote):

> >  How does 10% xfail rate sound?
@h-vetinari honestly this PR would take a significant amount of time to review and i don't know when I would be able to get to it. This  has a LOT of things going on, which need to be checked. Again reducing the scope of this would increase the likelihood that this would be merged.
> @jreback: @h-vetinari honestly this PR would take a significant amount of time to review and i don't know when I would be able to get to it. This has a LOT of things going on, which need to be checked. Again reducing the scope of this would increase the likelihood that this would be merged.

One, it's not introducing new functionality, just tests for it.
Two, I keep suggesting a pared-down version with "only" 10% xfails. Would that rate be (more) acceptable to you?
> I keep suggesting a pared-down version with "only" 10% xfails. Would that rate be (more) acceptable to you?

better than this one