TST: add test coverage for maybe_promote

[h-vetinari]

• first step for BUG/Internals: maybe_promote #23833
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• fixes a regression that was silently introduced in CLN/DEPS: Clean up post numpy bump to 1.12 #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 BUG/Internals: maybe_promote #23833).
• in CLN/DEPS: Clean up post numpy bump to 1.12 #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 TST/CLN: series.duplicated; parametrisation; fix warning #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:

>>> 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)

[pep8speaks]

Hello @h-vetinari! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

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

Comment last updated at 2019-10-11 13:29:07 UTC

[jreback]

this is not a good fixture name, nor correct as these are not times but datetimes and timedeltas which are very separate

[jreback]

fixes a regression that was silently introduced in #23796 due to lack of tests

what was the regression?

[h-vetinari]

@jreback

fixes a regression that was silently introduced in #23796 due to lack of tests

what was the regression?

The try/catch around these were removed due to ecfe824#r235838683, but the Timestamp/Timedelta constructor choke when fed with strings/objects.

[jreback]

what did this break from a user POV? if it didn't then don't revert this, it should fail.

[h-vetinari]

I didn't say it broke anything from a user perspective, just that #23796 broke something that wasn't tested (namely, trying to upcast from datetime/timedelta to string/object

[jreback]

so these should probably be using: is_datetime64_dtype(dtype) and is_timedelta64_dtype(dtype) as more consistent with the rest of the codebase.

I am also not sure whether much of this routine (maybe_promote) is actually used in a meaningful way. Hence I don't think these checks are needed currently.

[h-vetinari]

I am also not sure whether much of this routine (maybe_promote) is actually used in a meaningful way. Hence I don't think these checks are needed currently.

There's a list of places it appear in in #23833, but one of the key points is in maybe_upcast_putmask, which I want to use as the backend for .update in #23192, see #23606, #22358 etc.

[codecov]

Codecov Report

Merging #23982 into master will decrease coverage by 1.31%.
The diff coverage is n/a.

@@            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%)	⬇️
#single	41.69% <ø> (-0.75%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	25% <0%> (-75%)	⬇️
pandas/compat/__init__.py	58.1% <0%> (-36.77%)	⬇️
pandas/plotting/_misc.py	38.68% <0%> (-26.18%)	⬇️
pandas/io/common.py	72.86% <0%> (-21.25%)	⬇️
pandas/io/gcs.py	80% <0%> (-20%)	⬇️
pandas/io/s3.py	86.36% <0%> (-13.64%)	⬇️
pandas/io/excel/_util.py	78.82% <0%> (-11.18%)	⬇️
pandas/core/computation/expr.py	88.17% <0%> (-9.35%)	⬇️
pandas/core/groupby/base.py	91.83% <0%> (-8.17%)	⬇️
pandas/plotting/_core.py	83.58% <0%> (-6.05%)	⬇️
... and 197 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 558f754...0c5b524. Read the comment docs.

[h-vetinari]

@jreback
Merged in #23983 and green.

[jreback]

no
these never can be in the same fixture

[h-vetinari]

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.

[h-vetinari]

@jreback
Split things up into two fixtures and two x two tests now. All green too.

[h-vetinari]

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.

[h-vetinari]

@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.

[jreback]

we’re are way over for 0.24

changes will be merged if they can be accommodated

[h-vetinari]

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.

[jreback]

@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]

@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.

[jreback]

what did this break from a user POV? if it didn't then don't revert this, it should fail.

[jreback]

this is too big of an add here, make a new file. you move the existing tests to
pandas/tests/dtypes/cast/test_cast.py then add this. but needs to be split up this is too massive.

[h-vetinari]

The existing tests are already in pandas/tests/dtypes/cast/test_cast.py. Do you want me to make a new file pandas/tests/dtypes/cast/test_promote.py?

[jreback]

existing are in pandas/tests/dtypes/test_cast.py, so make a subdir 'cast' and move the original, then add new in test_promote in that subdir

[h-vetinari]

Thanks for reviewing

[h-vetinari]

I didn't say it broke anything from a user perspective, just that #23796 broke something that wasn't tested (namely, trying to upcast from datetime/timedelta to string/object

[h-vetinari]

The existing tests are already in pandas/tests/dtypes/cast/test_cast.py. Do you want me to make a new file pandas/tests/dtypes/cast/test_promote.py?

[h-vetinari]

Shifted the modules to a new subfolder as desired

[jreback]

adding 700 xfailed testsis not great here. simply comment them out for now. i know you actually want to promote based on the dtype fo the fill value, but to be honest numpy already does this and i don't think we actually need to do it. I would rather show where this is not working correctly first.

I actually want to completely remove this routine (or rather make it useful), so its possible all of these tests are needed. but would do some more investigating on the use (or lack thereof) of this routine first.

[jreback]

so these should probably be using: is_datetime64_dtype(dtype) and is_timedelta64_dtype(dtype) as more consistent with the rest of the codebase.

I am also not sure whether much of this routine (maybe_promote) is actually used in a meaningful way. Hence I don't think these checks are needed currently.

[jreback]

needs updating

[jreback]

this should be the same as the above case, we always promote to np.nan

[h-vetinari]

Yeah, this is a left-over from the currently very inconsistent behaviour of not sometimes returning the value of fill_value.

[h-vetinari]

@jreback

adding 700 xfailed tests is not great here. simply comment them out for now.

You always ask for focused PRs and "tests first", so I'm doing that here...

i know you actually want to promote based on the dtype fo the fill value, but to be honest numpy already does this and i don't think we actually need to do it.

Numpy is fraught with silent under- & overflow issues in the ints, and raising errors when safe casting is not possible. That's what I'm trying to solve here (again, I'm actually aiming at fixing maybe_upcast_putmask, but this method is used as an auxiliary function, and IMO worth fixing).

I would rather show where this is not working correctly first.

There's a long list of examples in the OP and #23833.

I actually want to completely remove this routine (or rather make it useful), so its possible all of these tests are needed. but would do some more investigating on the use (or lack thereof) of this routine first.

This is used is a few crucial places, so not sure how easy it would be to get rid of.

[h-vetinari]

I am also not sure whether much of this routine (maybe_promote) is actually used in a meaningful way. Hence I don't think these checks are needed currently.

There's a list of places it appear in in #23833, but one of the key points is in maybe_upcast_putmask, which I want to use as the backend for .update in #23192, see #23606, #22358 etc.

[h-vetinari]

Yeah, this is a left-over from the currently very inconsistent behaviour of not sometimes returning the value of fill_value.

[h-vetinari]

@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.

[h-vetinari]

The doc job had a 500 error - could someone restart it please? (@gfyoung ?)

[h-vetinari]

@jreback
After the recent CI woes are finally surmounted: PTAL. :)

[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.

[jbrockmendel]

nice, not much left, thanks

[h-vetinari]

Much closer, that's true! Thanks a lot for your work on this. There's however still a couple of places where the parametrization of box has been overridden due to xfails, and this is something that I couldn't fix right away when merging master today. I'll try to re-enable those as well...

[jbrockmendel]

There's however still a couple of places where the parametrization of box has been overridden due to xfails

Yah. My attempts to get the box=True cases working have been much less successful than the other cases, so current thought is to try to rule out their necessity (pretty sure we discussed this elsewhere).

Aside from rebasing this and the other one, if you'd like to stake out small areas to fix bit by bit, I'll try to keep focused on non-overlapping areas (currently only open PR on this is #28833)

[h-vetinari]

Yah. My attempts to get the box=True cases working have been much less successful than the other cases, so current thought is to try to rule out their necessity (pretty sure we discussed this elsewhere)

We discussed this here. I agree that it's tempting to want to rip out the array path, but then I'd say we also need a plan how this can be supported for cases where this is or will be needed (like some of the maybe_upcast_putmask and update stuff). I think there's some value in only having the promotion code once (but then again, we're far from that already, as I sketched in the linked comment)

Aside from rebasing this and the other one, if you'd like to stake out small areas to fix bit by bit, I'll try to keep focused on non-overlapping areas (currently only open PR on this is #28833)

Currently I'm a bit swamped, so not sure when I'll get to it. Hopefully on the weekend.

[h-vetinari]

@jbrockmendel
I guess by now you chopped off everything but the parametrization of one of the bytes-tests. I guess that makes this PR mergeable? 😅

[jreback]

@h-vetinari just merged a few things, can you rebase once again, otherwise lgtm.

[h-vetinari]

@jreback
Merged and green. :)
Can't believe this one's finally finished, haha.

[jreback]

@jreback
Merged and green. :)
Can't believe this one's finally finished, haha.

haha thank @jbrockmendel

[h-vetinari]

haha thank @jbrockmendel

Indeed. :)

[jbrockmendel]

alright @h-vetinari time to figure out the boxed cases. want to propose a gameplan?

[h-vetinari]

@jbrockmendel: alright @h-vetinari time to figure out the boxed cases. want to propose a gameplan?

The first and most important decision is if we want to support the array case with maybe_promote. It's tempting to want to rip it out due to the complexity, but it also leads to more complexity down the road elsewhere when we want to fix longstanding things like the wrong upcasting in .update (which needs a sort of array-case, because we definitely don't want to loop through the entries), cf. #23606 for one of many examples.

I think it's beneficial to keep the logic in one place (or at least fewer places: there's similar things happening with maybe_convert_objects in lib that does very similar logic, and should be consolidated eventually; also for perf through cython). See comment here and the reference therein.

If we want to keep the array-case, then my first suggestion would be something like I did in #25425 - split the cases into maybe_promote_with_scalar and maybe_promote_with_array, but unify the actual promotion logic in one place (maybe an underlying method? in my case I used maybe_promote_with_array as the underlying directly). This would also allow to easily refactor the whole code base to adapt each call-site with the appropriate scalar- or array-case.

If the decision is to not support the array case, we'd just have to rip out or xfail some maybe_upcast_putmask tests and remove the respective branches from maybe_promote and the tests and call it a day. It would be a pyrrhic victory though, IMO, at least without a plan how to handle arrays. However, it could be that the total effort is lower for ripping out the array-case and starting from scratch with something else, rather than trying to fix the existing method.

[jbrockmendel]

This seems like a good time to refactor out something like maybe_promote_ndarray. PR welcome.

Another bite-size PR that would be welcome would be adding a good docstring.

Do you have fixes in mind for the remaining xfailed cases? bite-sized PRs for those would be welcome.

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?