TST: tests for maybe_promote (precursor to #23982)
State: closed

First step towards #23833, resp. precursor to #23982.

TL;DR: `maybe_promote` is quite broken. #23982 tries to come up with tests that it should pass, and #25425 tries to fix the implementation. However, #23982 is quite big, so @jreback asked for a smaller version that (mostly) just tests existing behaviour.

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff

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-06-21 06:15:45 UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25637?src=pr&el=h1) Report
> Merging [#25637](https://codecov.io/gh/pandas-dev/pandas/pull/25637?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/26cfa288edfc2db85d23d3e23cb6ce3888675963?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #25637      +/-   ##
==========================================
+ Coverage   91.26%   91.27%   +<.01%     
==========================================
  Files         173      173              
  Lines       52968    52968              
==========================================
+ Hits        48339    48344       +5     
+ Misses       4629     4624       -5
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.84% <ø> (ø)` | :arrow_up: |
| #single | `41.71% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25637?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/25637/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `89% <0%> (+0.83%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25637?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/25637?src=pr&el=footer). Last update [26cfa28...ca8f96b](https://codecov.io/gh/pandas-dev/pandas/pull/25637?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/25637?src=pr&el=h1) Report
> Merging [#25637](https://codecov.io/gh/pandas-dev/pandas/pull/25637?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b9b081dc6b510c8290ded12fe751b1216843527e?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #25637      +/-   ##
==========================================
- Coverage   91.97%   91.97%   -0.01%     
==========================================
  Files         180      180              
  Lines       50756    50756              
==========================================
- Hits        46685    46683       -2     
- Misses       4071     4073       +2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.57% <ø> (ø)` | :arrow_up: |
| #single | `41.84% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25637?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25637/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `88.88% <0%> (-11.12%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25637/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.89% <0%> (-0.12%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25637/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.94% <0%> (+0.1%)` | :arrow_up: |
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/25637/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `90.69% <0%> (+0.16%)` | :arrow_up: |

------

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

though a good idea, closing as stale
> @jreback: though a good idea, closing as stale

I've worked an inordinate amount of time on #25425, and although I prepared two subsets of it (#23982 and this one), it never got any review. I understand the realities of how scarce reviewing resources are, but it's still disheartening after putting in so much work.

Long story short, all those three PRs (#23982,#25425, #25637) should be reopened, please. I'm ready to keep working on them.
@h-vetinari : I'll re-open this one first.  I agree with @jreback that the scope of what you're trying to do is massive.  Thus, it would be great if you focused on this PR alone.

We can re-open other ones as the work progresses.
> it would lead to huge amount of duplication to separate testing the scalar case vs. the array case.

In a separate commit, separate these out as requested.  The best way to make the point here is to show the code side-by-side.  Are you sure you can't abstract out setup methods for some of the functionality that would be de-duplicated by any chance?

Also, if you could address the merge conflict.
@gfyoung 
Thanks for reopening.

> In a separate commit, separate these out as requested.

I can do this, but essentially *every* test would be duplicated. I'm testing the same logic in both cases, and actually more than that: *that* the same promotion logic applies in both scalar and array case.

If I start splitting this, it would make most sense to have one test for scalars and one for arrays, but from that point on, it would be very easy for those tests to diverge. Does this sound like a reasonable trade-off to you? While trivial, it will be a fair bit of work to actually duplicate these, so I'd rather be sure you understand the consequences before-hand.

>  Are you sure you can't abstract out setup methods for some of the functionality that would be de-duplicated by any chance?

That's exactly what I was aiming for with `_check_promote`. All the "differences" between the array and scalar case are taken care of by the wrapper, so that each test *only* has to specify the promotion logic that's being tested.
> If I start splitting this, it would make most sense to have one test for scalars and one for arrays, but from that point on

Why don't you try duplicating for one test only.  Then we can evaluate before and after on that one test and see whether it makes sense for you to continue forward with the duplication.

> That's exactly what I was aiming for with _check_promote.

Hmm...I see.  It sounds though (from the comments IIUC) that that might have impacted readability because there were so many different cases to check.  Is it possible to maybe have two or three different check promote functions that each handle a subset of cases?
@gfyoung 

Finally got around to coming back to this:

> > That's exactly what I was aiming for with _check_promote.

> Hmm...I see. It sounds though (from the comments IIUC) that that might have impacted readability because there were so many different cases to check. Is it possible to maybe have two or three different check promote functions that each handle a subset of cases?

I added a docstring and some comments to make it clearer. The function essentially only contains one branch depending on whether `fill_value` should be passed as scalar or as an array. And that's exactly the part I want to abstract away from the tests further on.

All of those tests have a very simple format:
```python
def test_maybe_promote_XXX_with_YYY(fixtures..., boxed, box_dtype):
    dtype = some_dtype(XXX)
    fill_dtype = some_dtype(YYY)

    if some_condition:
        pytest.xfail('reason')

    # define fill_value (usually implicitly through fill_dtype)
    fill_value = some_scalar(fill_dtype)

    # define expected values
    expected_dtype = some_dtype
    exp_val_for_scalar = some_scalar_value
    exp_val_for_array = some_missing_value_marker

    _check_promote(dtype, fill_value, boxed, box_dtype, expected_dtype,
                   exp_val_for_scalar, exp_val_for_array)
```

This about as simple as I can imagine it to be (while unifying the treatment of scalar and array case). Short of a new idea (and the docstring), I don't know how to make this clearer.

(speaking of new ideas: I incorporated your typecheck instead of the try/except for `PandasExtensionDtype`)
> @gfyoung: [Incorporate this side-note as a comment. That will help to explain the parameterized values better.](https://github.com/pandas-dev/pandas/pull/25637/files#r289039845)

I started adding more explanations, but since that would have been repeated for all tests, I decided it's cleaner to have a parametrized fixture that only needs to be explained once (and is overridden where necessary due to xfails or special cases).

I think this is much cleaner now. :)
@gfyoung 
This is now slightly reduced in size after #26596. ~Also, could you please restart the azure CI?~

Edit: Nevermind, I had some linting to commit anyway.
@jreback : Could you have another look at this
at some point 

so why are you haveing *every* int type against a timedelta64 for example; this is just way overkill. These should all react the same, but having this expanse of tests is just too confusing. Please limit this.

Obviously the same holds true for example datetime.

```
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint8-timedelta64[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint8-timedelta64[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint8-m8[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint8-m8[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint16-timedelta64[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint16-timedelta64[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint16-m8[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint16-m8[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint32-timedelta64[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint32-timedelta64[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint32-m8[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint32-m8[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint64-timedelta64[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint64-timedelta64[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint64-m8[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[uint64-m8[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int-timedelta64[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int-timedelta64[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int-m8[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int-m8[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int8-timedelta64[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int8-timedelta64[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int8-m8[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int8-m8[ns]-box2-np.timedelta64]
  reason: does not upcast correctly
XFAIL pandas/tests/dtypes/cast/test_promote.py::test_maybe_promote_any_with_timedelta64[int16-timedelta64[ns]-box0-np.timedelta64]
  reason: does not upcast correctly
```
> @jreback: so why are you haveing every int type against a timedelta64 for example; this is just way overkill. These should all react the same, but having this expanse of tests is just too confusing. Please limit this.

The simple answer is: fixtures - they do exactly what I wanted here: exhaustively (as possible) test every type against (inputs of) every other type. I get the argument that some tests can be overkill, but I can't see why it would be confusing...?

Are you opposed specifically to the number of xfails, I'm guessing? Otherwise, once one starts introducing special cases in this module it becomes less understandable, more fragile, and more complex.
> The simple answer is: fixtures - they do exactly what I wanted here: exhaustively (as possible) test every type against (inputs of) every other type. I get the argument that some tests can be overkill, but I can't see why it would be confusing...?

@h-vetinari this add quite a bit of duplicative testing for many types. Please use more selective fixtures. If its obvious that something is doing the same exact cast vs int, int8, int16.....then a single int is enough for a fixture with a nice comment.

@jreback 
What do you think of the reduced fixture I introduced following your review?
ok @h-vetinari whats 3000+ new tests, lol.

merge master once again and ping on green.
> merge master once again and ping on green.

@jreback, this is green. :)
k thanks @h-vetinari 
@jreback @gfyoung 
Thanks for reviewing this big hunk. :)

> We can re-open other ones as the work progresses.

Can you please reopen #25425?

I'd also suggest that we reopen #23982 anyway - I don't think it has a chance of getting merged (because it basically just adds a bunch of failing tests), but once rebased it would provide a good insight for the _value_ of the refactor #25425 - as in: all the things that are currently broken and would be fixed by that PR.
@jreback @gfyoung 

> @h-vetinari: Can you please reopen #25425?