API: Validate keyword arguments to fillna #19684

TomAugspurger · 2018-02-13T20:13:08Z

Closes #19682

Closes pandas-dev#19682

TomAugspurger · 2018-02-13T20:40:59Z

pandas/core/arrays/categorical.py

@@ -1607,6 +1607,9 @@ def fillna(self, value=None, method=None, limit=None):
        -------
        filled : Categorical with NA/NaN filled
        """
+        value, method = validate_fillna_kwargs(


Note, I added this keyword since it's possible have a tuple / list for a category.

Do we need to add a test for this? (I mean filling a categorical with such categories, so the purpose of validate_scalar_dict_value=False is exercised)

jorisvandenbossche · 2018-02-13T20:51:27Z

pandas/util/_validators.py

+    if value is None and method is None:
+        raise ValueError("Must specify a fill 'value' or 'method'.")
+    elif value is None and method is not None:
+        clean_fill_method(method)


method = clean_fill_method(method) ?

Yeah, thought about it. Meant to double check one thing in categorical but forgot. One sec...

gfyoung · 2018-02-14T04:28:01Z

pandas/util/_validators.py

+    Parameters
+    ----------
+    value, method : object
+        The 'value' and 'method' keyword arguments for 'fillna'.


What happened to validate_scalar_dict_value in the docstring?

codecov · 2018-02-14T11:29:06Z

Codecov Report

Merging #19684 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #19684      +/-   ##
==========================================
+ Coverage   91.61%   91.61%   +<.01%     
==========================================
  Files         150      150              
  Lines       48892    48900       +8     
==========================================
+ Hits        44792    44800       +8     
  Misses       4100     4100

Flag	Coverage Δ
#multiple	`89.99% <100%> (ø)`	⬆️
#single	`41.79% <62.5%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/generic.py	`95.93% <100%> (-0.01%)`	⬇️
pandas/util/_validators.py	`96.8% <100%> (+0.46%)`	⬆️
pandas/core/arrays/categorical.py	`94.91% <100%> (ø)`	⬆️

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 aa59954...dc1f960. Read the comment docs.

jreback · 2018-02-15T12:36:18Z

pandas/tests/categorical/test_missing.py

+        cat = Categorical([1, 2, 3])
+
+        xpr = "Cannot specify both 'value' and 'method'."
+


can you remove the line between the message and the condition, personal preference but its slightly distracting

TomAugspurger · 2018-02-16T13:04:56Z

Merging later today if no objections.

jreback · 2018-02-16T13:27:37Z

let me look

TomAugspurger · 2018-02-16T13:32:26Z

pandas/util/_validators.py

+        method = clean_fill_method(method)
+
+    elif value is not None and method is None:
+        if validate_scalar_dict_value and isinstance(value, (list, tuple)):


This check doesn't seem especially useful / correct given the error message.

There are many things that are not scalars or dicts, but also not lists or tuples, so we would fail to hit this TypeError.

But I was trying to keep this as a straight refactor for NDFrame.fillna, with the only behavior change being Categorical.fillna.

I dont' think this is odd to want to validate that a scalar can be only a certain type. maybe would change this

validate_scalar=lambda x: is_scalar(x) or is_dict_like(x)

this parameter name is terrible, ideally would like to change this

Two things:

Please don't call a change by a contributor "terrible". It's not helpful.

The error message says "'value' parameter must be a scalar or dict". That seems to match the parameter name pretty well.

the parameter name is terrible, obviously NOT the contributor, which I didn't say or imply at all. It needs to be changed.

The parameter name is certainly descriptive in what it does, and it is for an internal method, so it's not much of a problem it is that long. Why bother for the rest?

gfyoung · 2018-02-16T14:21:56Z

pandas/util/_validators.py

+    value, method : object
+        The 'value' and 'method' keyword arguments for 'fillna'.
+    validate_scalar_dict_value : bool, default True
+        Whether to validate that 'value' is a scalar or dict; specifically


Nit: replace the semicolon with a comma.

jreback · 2018-02-16T18:00:57Z

pandas/util/_validators.py

+        raise ValueError("Must specify a fill 'value' or 'method'.")
+    elif value is None and method is not None:
+        method = clean_fill_method(method)
+


extra line here (make consistent across this function)

jreback · 2018-02-16T18:15:25Z

pandas/util/_validators.py

+        method = clean_fill_method(method)
+
+    elif value is not None and method is None:
+        if validate_scalar_dict_value and isinstance(value, (list, tuple)):


I dont' think this is odd to want to validate that a scalar can be only a certain type. maybe would change this

validate_scalar=lambda x: is_scalar(x) or is_dict_like(x)

TomAugspurger · 2018-02-20T12:11:13Z

I was trying to add a test for cat.fillna(tuple), but apparently, we don't allow it anyway. On master.

In [1]: import pandas as pd

In [2]: pd.Categorical([(1, 2), None]).fillna((1, 2))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-73c5b4ab00c2> in <module>()
----> 1 pd.Categorical([(1, 2), None]).fillna((1, 2))

~/sandbox/pandas-ip/pandas/pandas/util/_decorators.py in wrapper(*args, **kwargs)
    136                 else:
    137                     kwargs[new_arg_name] = new_arg_value
--> 138             return func(*args, **kwargs)
    139         return wrapper
    140     return _deprecate_kwarg

~/sandbox/pandas-ip/pandas/pandas/core/arrays/categorical.py in fillna(self, value, method, limit)
   1664                 raise TypeError('"value" parameter must be a scalar, dict '
   1665                                 'or Series, but you passed a '
-> 1666                                 '"{0}"'.format(type(value).__name__))
   1667
   1668         return self._constructor(values, categories=self.categories,

TypeError: "value" parameter must be a scalar, dict or Series, but you passed a "tuple"

I've left the validate_scalar_dict_value boolean there, as prep for #19705, but can remove from here if you want.

jreback · 2018-02-20T13:40:06Z

doc/source/whatsnew/v0.23.0.txt

@@ -590,6 +590,8 @@ Other API Changes
 object frequency is ``None`` (:issue:`19147`)
 - Set operations (union, difference...) on :class:`IntervalIndex` with incompatible index types will now raise a ``TypeError`` rather than a ``ValueError`` (:issue:`19329`)
 - :class:`DateOffset` objects render more simply, e.g. "<DateOffset: days=1>" instead of "<DateOffset: kwds={'days': 1}>" (:issue:`19403`)
+- :func:`pandas.merge` provides a more informative error message when trying to merge on timezone-aware and timezone-naive columns (:issue:`15800`)


there is a datetimelike section for other api changes now

jreback · 2018-02-20T13:40:48Z

pandas/util/_validators.py

+        method = clean_fill_method(method)
+
+    elif value is not None and method is None:
+        if validate_scalar_dict_value and isinstance(value, (list, tuple)):


this parameter name is terrible, ideally would like to change this

…illna

jreback · 2018-02-22T00:21:01Z

thanks @TomAugspurger

API: Validate keyword arguments to fillna

bfb868b

Closes pandas-dev#19682

TomAugspurger commented Feb 13, 2018

View reviewed changes

jorisvandenbossche reviewed Feb 13, 2018

View reviewed changes

Return cleaned value

dddc7cf

gfyoung reviewed Feb 14, 2018

View reviewed changes

gfyoung added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Error Reporting Incorrect or improved errors from pandas labels Feb 14, 2018

Docstring

f810f67

Merge remote-tracking branch 'upstream/master' into validate-fillna

3919da4

jreback requested changes Feb 15, 2018

View reviewed changes

Parametrized

c280eaa

TomAugspurger commented Feb 16, 2018

View reviewed changes

gfyoung reviewed Feb 16, 2018

View reviewed changes

jreback requested changes Feb 16, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into validate-fillna

96c1754

TomAugspurger mentioned this pull request Feb 20, 2018

Categorical.fillna doesn't accept tuples #19788

Closed

jreback requested changes Feb 20, 2018

View reviewed changes

TomAugspurger added 4 commits February 20, 2018 08:02

Moved tz merge whatsnew

6c09f08

Docstring grammar

90f8fde

Merge remote-tracking branch 'upstream/master' into validate-fillna

38aeecf

fixup! Merge remote-tracking branch 'upstream/master' into validate-f…

dc1f960

…illna

jreback added this to the 0.23.0 milestone Feb 22, 2018

jreback approved these changes Feb 22, 2018

View reviewed changes

jreback merged commit 3b135c3 into pandas-dev:master Feb 22, 2018

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

API: Validate keyword arguments to fillna (pandas-dev#19684)

891ee92

TomAugspurger deleted the validate-fillna branch May 2, 2018 13:09

TomAugspurger mentioned this pull request May 22, 2018

Categorical fillna with custom objects raises TypeError #21097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Validate keyword arguments to fillna #19684

API: Validate keyword arguments to fillna #19684

TomAugspurger commented Feb 13, 2018

TomAugspurger Feb 13, 2018

jorisvandenbossche Feb 16, 2018

jorisvandenbossche Feb 13, 2018

TomAugspurger Feb 13, 2018

gfyoung Feb 14, 2018

codecov bot commented Feb 14, 2018 •

edited

Loading

jreback Feb 15, 2018

TomAugspurger commented Feb 16, 2018

jreback commented Feb 16, 2018

TomAugspurger Feb 16, 2018

jreback Feb 16, 2018

jreback Feb 20, 2018

TomAugspurger Feb 20, 2018 •

edited

Loading

jreback Feb 20, 2018

jorisvandenbossche Feb 20, 2018

gfyoung Feb 16, 2018

jreback Feb 16, 2018

jreback Feb 16, 2018

TomAugspurger commented Feb 20, 2018

jreback Feb 20, 2018

jreback Feb 20, 2018

jreback commented Feb 22, 2018

		cat = Categorical([1, 2, 3])

		xpr = "Cannot specify both 'value' and 'method'."

API: Validate keyword arguments to fillna #19684

API: Validate keyword arguments to fillna #19684

Conversation

TomAugspurger commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

TomAugspurger commented Feb 16, 2018

jreback commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 22, 2018

codecov bot commented Feb 14, 2018 •

edited

Loading

TomAugspurger Feb 20, 2018 •

edited

Loading