Repository: pandas-dev/pandas
Pull request: DOC: fixed doc-string for combine & combine_first in pandas/core/series.py
- [x] closes #22953
- [ ] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Problem statement:
DOC: fill_value argument in Series.combine()
Series.combine() method is not handling NaN values correctly.
On running this code, NaN is still returned and isnt replaced by 5.

```
#importing pandas module
import pandas as pd

#importing numpy module
import numpy as np

#creating first series
first=[1,2,np.nan,5,6,3,np.nan,7,11,0,4,8]

#creating second series
second=[5,3,2,np.nan,1,3,9,21,3,np.nan,1,np.nan]

#making series
first=pd.Series(first)

#making seriesa
second=pd.Series(second)

#calling .combine() method
result=first.combine(second, (lambda x1, x2: x1 if x1 > x2 else x2) , fill_value=5)

#display
result
```

The NULL values in Caller series are replaced but the Null values in the passed series remain there. Unlike other similar methods like .add(), .sub() etc. this one isn't handling Null values of passed series correctly.

If this issue is approved, i would like to contribute and fix it

Hints from issue discussion:
I would like to work this bug !
`>>> result`
0      5.0
1      3.0
2      2.0
3      NaN
4      6.0
5      3.0
6      9.0
7     21.0
8     11.0
9      NaN
10     4.0
11     NaN
dtype: float64
`>>> type(first[0])`
<class 'numpy.float64'>
`>>> first[0]`
1.0
`>>> first[0] > np.nan`
False

honestly, @kbhutan0001 , I don't know if we should even compare a `Not A Number` and a `float`
See this [answer from StackOverflow](https://stackoverflow.com/questions/1565164/what-is-the-rationale-for-all-comparisons-returning-false-for-ieee754-nan-values)
@brute4s99 We are not comparing, we are combining them. This bug is because the function is probably executed before na replacement and hence null is compared with numeric values

okay, @kbhutani0001 . I'll look more into this([.](https://github.com/pandas-dev/pandas/blob/v0.23.4/pandas/core/series.py#L2201-L2246)) tomorrow !
This is the correct behavior as the document states:

> optional fill value when an **index** is missing from one Series or the other

so `fill_value` represents the placeholder of the value corresponding to the index that doesn't exist not the `fill_value` of NaN values.

```
In [13]: second.loc[12] = 1

In [14]: result=first.combine(second, (lambda x1, x2: x1 if x1 > x2 else
    ...: x2) , fill_value=5)

In [15]: result
Out[15]:
0      5.0
1      3.0
2      2.0
3      NaN
4      6.0
5      3.0
6      9.0
7     21.0
8     11.0
9      NaN
10     4.0
11     NaN
12     5.0 # 5 > 1 so we get 5 here
dtype: float64
```

Would welcome an example in the docstring showing this behavior.
In other mathematical operation, fill_na fills NULL values in both series
so i don't think this one should be like this. Moreover what's the point of
null handling when you get NaN in output series anyway. Either don't do it
or do it for both😅

On Wed, 3 Oct 2018, 1:58 am Matthew Roeschke, <notifications@github.com>
wrote:

> This is the correct behavior as the document states:
>
> optional fill value when an *index* is missing from one Series or the
> other
>
> so fill_value represents the placeholder of the value that doesn't exist
> in the index not the fill_value of NaN values.
>
> In [13]: second.loc[12] = 1
>
> In [14]: result=first.combine(second, (lambda x1, x2: x1 if x1 > x2 else
>     ...: x2) , fill_value=5)
>
> In [15]: result
> Out[15]:
> 0      5.0
> 1      3.0
> 2      2.0
> 3      NaN
> 4      6.0
> 5      3.0
> 6      9.0
> 7     21.0
> 8     11.0
> 9      NaN
> 10     4.0
> 11     NaN
> 12     5.0 # 5 > 1 so we get 5 here
> dtype: float64
>
> Would welcome an example in the docstring showing this behavior.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/issues/22953#issuecomment-426418015>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/Ae0Do9ZcAVoiT6bSv4QcXvhOBsYOYFbuks5ug8xvgaJpZM4XEwvC>
> .
>

The names of the arguments are slightly different `fill_na` vs `fill_value` (though `fill_value` isn't a great name for what it does).

Plus all comparisons with `np.nan` are False, so the given function `lambda x1, x2: x1 if x1 > x2 else x2`, `np.nan` comparisons will always fill with the 2nd value. 
> Would welcome an example in the docstring showing this behavior.

on it !
> The names of the arguments are slightly different `fill_na` vs `fill_value` (though `fill_value` isn't a great name for what it does).

how about `fallback_value` , @mroeschke ?

Commit message: Added practical example

Patch to review:
diff --git a/pandas/core/series.py b/pandas/core/series.py
index 892b24f6ee552..1e96b7445f541 100644
--- a/pandas/core/series.py
+++ b/pandas/core/series.py
@@ -3,6 +3,10 @@
 """
 from __future__ import division
 
+# pylint: disable=E1101,E1103
+# pylint: disable=W0703,W0622,W0613,W0201
+
+import warnings
 from textwrap import dedent
 import warnings
 
@@ -2281,36 +2285,62 @@ def _binop(self, other, func, level=None, fill_value=None):
 
     def combine(self, other, func, fill_value=None):
         """
-        Perform elementwise binary operation on two Series using given function
-        with optional fill value when an index is missing from one Series or
-        the other
+        Combine the Series with a Series or scalar according to `func`.
+
+        Combine the Series and `other` using `func` to perform elementwise
+        selection for combined Series.
+        `fill_value` is assumed when value is missing at some index
+        from one of the two objects being combined.
 
         Parameters
         ----------
-        other : Series or scalar value
+        other : Series or scalar
+            The value(s) to be combined with the `Series`.
         func : function
-            Function that takes two scalars as inputs and return a scalar
-        fill_value : scalar value
-            The default specifies to use the appropriate NaN value for
-            the underlying dtype of the Series
+            Function that takes two scalars as inputs and returns an element.
+        fill_value : scalar, optional
+            The value to assume when an index is missing from
+            one Series or the other. The default specifies to use the
+            appropriate NaN value for the underlying dtype of the Series.
 
         Returns
         -------
-        result : Series
+        Series
+            The result of combining the Series with the other object.
+
+        See Also
+        --------
+        Series.combine_first : Combine Series values, choosing the calling
+                               Series' values first.
 
         Examples
         --------
         >>> s1 = pd.Series([1, 2])
-        >>> s2 = pd.Series([0, 3])
-        >>> s1.combine(s2, lambda x1, x2: x1 if x1 < x2 else x2)
-        0    0
-        1    2
+        >>> s2 = pd.Series([0, 3, 4])
+        >>> s1.combine(s2, lambda x1, x2: x1 if x1 > x2 else x2)
+        0    1
+        1    3
+        2    4
+        dtype: int64
+
+        >>> arms = pd.Series({'dog':2,'cat': 2,'mouse': 2})
+        >>> legs = pd.Series({'dog':2,'cat': 2})
+        >>> arms
+        dog      2
+        cat      2
+        mouse    2
+        dtype: int64
+        >>> legs
+        dog    2
+        cat    2
+        dtype: int64
+        >>> limbs = arms.combine(legs, lambda x1,x2: x1+x2, fill_value=0)
+        >>> limbs
+        cat      4
+        dog      4
+        mouse    2
         dtype: int64
 
-        See Also
-        --------
-        Series.combine_first : Combine Series values, choosing the calling
-            Series's values first.
         """
         if fill_value is None:
             fill_value = na_value_for_dtype(self.dtype, compat=False)
@@ -2352,16 +2382,26 @@ def combine(self, other, func, fill_value=None):
 
     def combine_first(self, other):
         """
-        Combine Series values, choosing the calling Series's values
-        first. Result index will be the union of the two indexes
+        Combine Series values, choosing the calling Series's values first.
 
         Parameters
         ----------
         other : Series
+            The value(s) to be combined with the `Series`.
 
         Returns
         -------
-        combined : Series
+        Series
+            The result of combining the Series with the other object.
+        
+        See Also
+        --------
+        Series.combine : Perform elementwise operation on two Series
+                         using a given function
+
+        Notes
+        -----
+        Result index will be the union of the two indexes.
 
         Examples
         --------
@@ -2371,11 +2411,7 @@ def combine_first(self, other):
         0    1.0
         1    4.0
         dtype: float64
-
-        See Also
-        --------
-        Series.combine : Perform elementwise operation on two Series
-            using a given function.
+        
         """
         new_index = self.index.union(other.index)
         this = self.reindex(new_index, copy=False)


Code review comments:
diff_hunk: @@ -2281,36 +2285,62 @@ def _binop(self, other, func, level=None, fill_value=None):
 
     def combine(self, other, func, fill_value=None):
         """
-        Perform elementwise binary operation on two Series using given function
-        with optional fill value when an index is missing from one Series or
-        the other
+        Combine the Series with a Series or scalar according to `func`.
+
+        Combine the Series and `other` using `func` to perform elementwise
+        selection for combined Series.
+        `fill_value` is assumed when value is missing at some index
+        from one of the two objects being combined.
 
         Parameters
         ----------
-        other : Series or scalar value
+        other : Series or scalar
+            The value(s) to be combined with the `Series`.
         func : function
-            Function that takes two scalars as inputs and return a scalar
-        fill_value : scalar value
-            The default specifies to use the appropriate NaN value for
-            the underlying dtype of the Series
+            Function that takes two scalars as inputs and returns an element.
+        fill_value : scalar, optional
+            The value to assume when an index is missing from
+            one Series or the other. The default specifies to use the
+            appropriate NaN value for the underlying dtype of the Series.
 
         Returns
         -------
-        result : Series
+        Series
+            The result of combining the Series with the other object.
+
+        See Also
+        --------
+        Series.combine_first : Combine Series values, choosing the calling
+                               Series' values first.
 
         Examples
         --------
         >>> s1 = pd.Series([1, 2])
-        >>> s2 = pd.Series([0, 3])
-        >>> s1.combine(s2, lambda x1, x2: x1 if x1 < x2 else x2)
-        0    0
-        1    2
+        >>> s2 = pd.Series([0, 3, 4])
+        >>> s1.combine(s2, lambda x1, x2: x1 if x1 > x2 else x2)
+        0    1
+        1    3
+        2    4
+        dtype: int64
+
+        >>> arms = pd.Series({'dog':2,'cat': 2,'mouse': 2})
+        >>> legs = pd.Series({'dog':2,'cat': 2})
+        >>> arms
+        dog      2
+        cat      2
+        mouse    2
path: pandas/core/series.py
text: @user1:
I'd say all those animals have 4 legs and 0 arms. Use correct data, and different values (spider, monkey...)

@author:
Does this example look fine?
```
>>> arms = pd.Series({'starfish': 5, 'kangaroo': 2})
>>> legs = pd.Series({'dog': 4,'kangaroo': 3})
>>> arms.combine(legs, lambda x, y: x+y, fill_value=0)
dog         4
kangaroo    5
starfish    5
dtype: int64
>>> arms.combine(legs, lambda x, y: x+y, fill_value=10)
dog         14
kangaroo     5
starfish    15
dtype: int64
>>>
```

@author:
I could not find any alternative to defining a `lambda`, since the next alternative was to use `+` itself, which cannot be done as a function param afaik

@user1:
Besides the example not following pep8 again, you may not be able to use a sum without a `lambda`, but as I said, you can use the builtin `max` function.

In the first example, remove completely the `fill_value` param, and in the second use a example that makes sense.

Also, add a short description before each case, explaining what is the goal of what you're showing. pandas does not contain functions or methods that are not useful for real-world examples. And in the documentation examples we should show those.

@author:
The purpose of combine() is to merge two Series into one.
So, I need 2 Series with some conflicting values[that use `func` to resolve conflict], and some NOT CONFLICTING ones[that could make use of `fill_value` to assume a value from the lacking Series]
I will use `max` for an example I just thought!

@author:
Consider 2 Datasets `data_A` and `data_B` containing highest clocked speeds of different birds.
```
>>> data_A = pd.Series({'Falcon': 330, 'Eagle': 160, 'Swift': 105, 'Duck': 30})
>>> data_B = pd.Series({'Falcon': 345, 'Eagle': 200, 'Swift': 100})
```
Now, to combine the two datasets and view the highest speeds of the birds across the
two datasets
```
>>> data_A.combine(data_B, max)
Duck       30
Eagle     200
Falcon    345
Swift     105
dtype: int64
```
Considering an average value of birds' speed to be ~40 mph,
```
>>> data_A.combine(data_B, max, fill_value=40)
Duck       40
Eagle     200
Falcon    345
Swift     105
dtype: int64
>>>
```

@author:
will this one work, @user1 ? The "average speed" was assumed after reading [from here](https://web.stanford.edu/group/stanfordbirds/text/essays/How_Fast.html)

@user1:
That looks much better, yes. I'd name the Series `s1` and `s2` following our standards, and also have the animal names in all lower case.

And I don't quite understand the `fill_value` part. I guess I'm misunderstanding something, but if:
```
>>> max(float('NaN'), 25)
nan
```
I would expect that in the first example `duck` has a value of `nan`. And in that case I'd use `fill_value=0` to get the known value. I don't quite lack making up an average speed. But I guess the function doesn't work as I think.

Can you do a bit of research and move the examples to the PR, so I can add new comments in a review.

Thanks!

@author:
@user1 look! something funny!
```
>>> max(30,float('nan'))
30
>>> max(float('nan'),30)
nan
```

@user2:
@user1 `fill_value` is used as the default value for the missing key in the corresponding series _before_ the comparison is made.

So in the example above, since `data_B` doesn't have `Duck`, the comparison is made as if `data_B['Duck'] = fill_value = 40` so `max(data_B['Duck'], data_A['Duck']) = max(40, 30) = 40`

@author:
> I would expect that in the first example `duck` has a value of `nan`.

[This answer on StackOverflow](https://stackoverflow.com/questions/47788361/why-does-max-sometimes-return-nan-and-sometimes-ignores-it) clears this doubt for me!


@author:
@user1 please read this !

diff_hunk: @@ -3,6 +3,10 @@
 """
 from __future__ import division
 
+# pylint: disable=E1101,E1103
+# pylint: disable=W0703,W0622,W0613,W0201
+
+import warnings
path: pandas/core/series.py
text: @user1:
please remove all this

diff_hunk: @@ -2281,36 +2285,62 @@ def _binop(self, other, func, level=None, fill_value=None):
 
     def combine(self, other, func, fill_value=None):
         """
-        Perform elementwise binary operation on two Series using given function
-        with optional fill value when an index is missing from one Series or
-        the other
+        Combine the Series with a Series or scalar according to `func`.
+
+        Combine the Series and `other` using `func` to perform elementwise
+        selection for combined Series.
+        `fill_value` is assumed when value is missing at some index
+        from one of the two objects being combined.
 
         Parameters
         ----------
-        other : Series or scalar value
+        other : Series or scalar
+            The value(s) to be combined with the `Series`.
         func : function
-            Function that takes two scalars as inputs and return a scalar
-        fill_value : scalar value
-            The default specifies to use the appropriate NaN value for
-            the underlying dtype of the Series
+            Function that takes two scalars as inputs and returns an element.
+        fill_value : scalar, optional
+            The value to assume when an index is missing from
+            one Series or the other. The default specifies to use the
+            appropriate NaN value for the underlying dtype of the Series.
 
         Returns
         -------
-        result : Series
+        Series
+            The result of combining the Series with the other object.
+
+        See Also
+        --------
+        Series.combine_first : Combine Series values, choosing the calling
+                               Series' values first.
 
         Examples
         --------
         >>> s1 = pd.Series([1, 2])
-        >>> s2 = pd.Series([0, 3])
-        >>> s1.combine(s2, lambda x1, x2: x1 if x1 < x2 else x2)
-        0    0
-        1    2
+        >>> s2 = pd.Series([0, 3, 4])
+        >>> s1.combine(s2, lambda x1, x2: x1 if x1 > x2 else x2)
+        0    1
+        1    3
+        2    4
+        dtype: int64
+
+        >>> arms = pd.Series({'dog':2,'cat': 2,'mouse': 2})
+        >>> legs = pd.Series({'dog':2,'cat': 2})
+        >>> arms
+        dog      2
+        cat      2
+        mouse    2
+        dtype: int64
+        >>> legs
+        dog    2
+        cat    2
+        dtype: int64
+        >>> limbs = arms.combine(legs, lambda x1,x2: x1+x2, fill_value=0)
path: pandas/core/series.py
text: @user1:
Do not assign, show the result directly.

Show the example without `fill_value` first, and then add `fill_value=whatever` to show the difference.

Would be nice to avoid `lambda`, if `max` or `min` can make sense, better use those.

There are pep8 errors, `scripts/validate_scripts.py` reports them now, make sure there are no validation errors before pushing, we built that script to avoid reviewing things that a script can tell you.