ENH: standardize fill_value behavior across the API

[ResidentMario]

Problem

In the PR for #15486, I found that type validation for the fill_value parameters strewn across a large number of pandas API methods is done ad-hoc. This results in a wide variety of possible accepted inputs. I think it would be good to standardize this so that all of these methods use the same behavior, the one currently used by fillna.

Implementation Details

Partially the point of providing a fill_value is to avoid having to do a slow-down type conversion otherwise (using .fillna().astype()). However, specifying other formats is nevertheless a useful convenience to have. Implementation would roughly be:

Before executing the rest of the method body, check whether or not the fill_value is valid (using a centralized maybe_fill method). If it is not, throw a ValueError. If it is, check whether or not incorporating the fill_value would result in an upcast in the column dtype. If it would not, follow a code path where the column never gets type-converted. If it would, follow that same code path, then do something like a filla operation at the end before returning.

Target Implementation

The same as what fillna currently does. Which follows.

Invalid:

• categorical fill for a category not in the categories will raise a ValueError.
• sparse matrices refuse upcasting.
• Passing an object or list or other non-coercable "thing" as a fill.

Valid, upcast:

• int fill will promote bool dtypes to int.
• float fill will promote int and bool dtypes to float (this is what happens with np.nan already).
• object (str) fill would promote lesser dtypes to object.
• int, float, and bool fill to a datetime dtype will be treated as a UNIX-like timestamp and promoted to datetime.
• object fill will promote datetime dtype to object.

Valid, no-cast:

• Everything else.

Current Implementation

...is ad-hoc. The following are the methods which currently provide a fill_value input, as well as where they deviate from the model above.

• Series.combine, DataFrame.combine, Series.to_sparse: These are unique usages of fill_value which aren't compatible with the rest of them.

• Series.unstack, DataFrame.unstack: any fill_value is allowed. You can pass an object if you'd like, or even another DataFrame (yo dawg...).

• DataFrame.align: Any fill_value is allowed.

• DataFrame.reindex_axis: Lists and dicts are allowed, objects are not.

• DataFrame.asfreq, Series.asfreq: any fill_value is allowed.

• pd.pivot_table: ...

• Series.add, DataFrame.add: ...

• Series.subtract, DataFrame.substract: ...

• Probably others, there's a lot of these.

[jreback]

this is kind of like _maybe_promote, see here: https://github.com/pandas-dev/pandas/blob/master/pandas/types/cast.py#L230, though in this case its a validator, but same idea.

a lot of the validation is prob occuring now but at a lower level and with no consistency of error messages. A lot of the routines expect certain types for filling, IOW filling floats needs a compatible float/int (or would raise).

So a friendly high level check would be nice. The hard part about this issue is not the code changes, but the tests :>

Also collecting these tests into a standard place would be fine as well (this is tricky because we like to keep the with the types, e.g. in pandas/tests/series, but for example routines in pandas/tests/missing would be nice as well.

[ResidentMario]

I'm hopeful I can figure out how to implement. Why are the tests the hard part? I assume you mean figuring out where to put them, which does sound challenging...might collect them in a separate file instead, just for the meantime.

[ResidentMario]

_maybe_upcast doesn't have guards against upcasting "weird" stuff. So for example the following is legal:

_maybe_upcast(np.array([np]))

When a fill_value parameter is passed, _maybe_upcast is the first and only validation step that parameter has to go through. So since the above is legal, so is whatever garbage you pass it, e.g. Series.shift(fill_value=<class 'garbage_type'>).

In other cases (e.g. fillna) there is additional validation that prevents this from happening.

Should _maybe_upcast (continue to) allow this behavior? This is deep in the internals, so I suspect not touching it would be best, but it does seem like an odd thing to allow, to me.

It wouldn't be too hard to add a separate check to prevent this sort of input from reaching _maybe_upcast at all.

[ResidentMario]

(sorry about the close/open, fat-fingered the wrong button there)

[jreback]

you might be able to add a check here
though i suspect have a _maybe_cast_fill which does validation might be easier

internal routines just have implicit (or better yet explicit guarantees that are in the docstring)

[ResidentMario]

FYI, this is legal in fillna right now:

pd.Series([1, 2, np.nan]).fillna(lambda f: f)

Which is counter-factual w.r.t a (separate) TypeError statement in the method body:

    if isinstance(value, (list, tuple)):
        raise TypeError('"value" parameter must be a scalar or dict, but '
                        'you passed a "{0}"'.format(type(value).__name__))

(to fix this you could do if isinstance(value, (list, tuple)) or callable(value))

[jreback]

yeah you prob have to check inclusion rather than exclusion

e.g. is_scalar, is_dict_like, is_list_like