DOC: fixed doc-string for combine & combine_first in pandas/core/series.py
State: closed

- [x] closes #22953
- [ ] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Docs

Comments:
Hello @brute4s99! Thanks for updating the PR.

 - There are no PEP8 issues in the file [`pandas/core/series.py`](https://github.com/brute4s99/pandas/blob/7902672ef4abe21e95b82ddd53826a23f0862672/pandas/core/series.py) !



##### Comment last updated on November 20, 2018 at 22:39 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22971?src=pr&el=h1) Report
> Merging [#22971](https://codecov.io/gh/pandas-dev/pandas/pull/22971?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/029d57cb828b053d112ed4e23f446ee07d147935?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/22971/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22971?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22971      +/-   ##
==========================================
+ Coverage   92.28%   92.29%   +<.01%     
==========================================
  Files         161      161              
  Lines       51457    51493      +36     
==========================================
+ Hits        47489    47524      +35     
- Misses       3968     3969       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.68% <ø> (ø)` | :arrow_up: |
| #single | `42.32% <ø> (+0.01%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22971?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/22971/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.68% <ø> (ø)` | :arrow_up: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/22971/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `89.18% <0%> (-0.08%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/22971/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.48% <0%> (ø)` | :arrow_up: |
| [pandas/core/resample.py](https://codecov.io/gh/pandas-dev/pandas/pull/22971/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzYW1wbGUucHk=) | `96.99% <0%> (ø)` | :arrow_up: |
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/22971/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `88.99% <0%> (+0.07%)` | :arrow_up: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/22971/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `96.44% <0%> (+0.34%)` | :arrow_up: |

------

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

@mroeschke please review
@brute4s99 can you update based on the previous review?
Sure, just a minute

@brute4s99 will you have time to address @datapythonista's comments?
>
>
I am so sorry, @datapythonista

I totally forgot about this PR.
Will push requested changes in an hour.

Can you take a closer look at the document I sent. I think `Examples` is the last section there.
> Can you take a closer look at the document I sent. I think `Examples` is the last section there.

oh, darn it! Sorry, @datapythonista 
but @datapythonista , The order says See Also -> Notes -> Examples
But we have Examples -> See Also in the codebase everywhere
So, where should I put `Notes` ?
Not sure about that everywhere. The link is the standard we follow, if there is any docstring that doesn't follow that order, it'll be eventually changed. And we'll soon also fail the CI if the order is wrong (see #23245).
Got it. I'll follow the link too then.
On it

On Tue, 20 Nov, 2018, 6:10 PM Marc Garcia, <notifications@github.com> wrote:

> *@datapythonista* requested changes on this pull request.
>
> Still some changes needed.
> ------------------------------
>
> In pandas/core/series.py
> <https://github.com/pandas-dev/pandas/pull/22971#discussion_r234984749>:
>
> > @@ -2289,6 +2289,11 @@ def combine(self, other, func, fill_value=None):
>          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.
>
> The indentation is wrong, should be indented 4 spaces respect to the
> previous line.
> ------------------------------
>
> In pandas/core/series.py
> <https://github.com/pandas-dev/pandas/pull/22971#discussion_r234984994>:
>
> >          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, default None
>
> ⬇️ Suggested change
>
> -        fill_value : scalar, default None
> +        fill_value : scalar, optional
>
> ------------------------------
>
> In pandas/core/series.py
> <https://github.com/pandas-dev/pandas/pull/22971#discussion_r234985071>:
>
> >          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, default None
> +            The optional value to assume when an index is missing from
>
> ⬇️ Suggested change
>
> -            The optional value to assume when an index is missing from
> +            The value to assume when an index is missing from
>
> ------------------------------
>
> In pandas/core/series.py
> <https://github.com/pandas-dev/pandas/pull/22971#discussion_r234985220>:
>
> > @@ -2292,10 +2303,22 @@ def combine(self, other, func, fill_value=None):
>          1    2
>          dtype: int64
>
> can you update this?
> ------------------------------
>
> In pandas/core/series.py
> <https://github.com/pandas-dev/pandas/pull/22971#discussion_r234985506>:
>
> >
>          Parameters
>          ----------
>          other : Series
> +            The value(s) to be combined with the `Series`.
>
>          Returns
>          -------
>          combined : Series
>
> ⬇️ Suggested change
>
> -         combined : Series
> +        Series
>
> Also add a description of what is being returned.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22971#pullrequestreview-176743797>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AV7mMyuhCVLbQo1aJK0cYGMGEwvvS_N-ks5uw_g8gaJpZM4XFnkv>
> .
>

also, this needs a rebase
Will do it right this time 😂😅

On Tue, 20 Nov, 2018, 6:15 PM Marc Garcia, <notifications@github.com> wrote:

> also, this needs a rebase
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22971#issuecomment-440260820>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AV7mM_o9vgKrXttYXmkENlhMXKYs_luFks5uw_mBgaJpZM4XFnkv>
> .
>

thinking of an example atm

Please review.
@datapythonista if `fill_value` is specified, the comparison is _guaranteed_ not to be made against `NaN` (unless `fill_value=np.nan`). `fill_value` fills the nan from the missing key _before_ the comparison is made i.e. the order of comparison doesn't matter.

```
In [3]: pd.__version__
Out[3]: '0.24.0.dev0+1069.gdf5eeece8'

In [4]: data_B = pd.Series({'Falcon': 345, 'Eagle': 200, 'Swift': 100})

In [5]: data_A = pd.Series({'Falcon': 330, 'Eagle': 160, 'Swift': 105, 'Duck': 30})

# data_A.combine(data_B, ...) == data_B.combine(data_A, ...)
In [6]: data_A.combine(data_B, max, fill_value=40)
Out[6]:
Duck       40
Eagle     200
Falcon    345
Swift     105
dtype: int64

In [7]: data_B.combine(data_A, max, fill_value=40)
Out[7]:
Duck       40
Eagle     200
Falcon    345
Swift     105
dtype: int64
```
@mroeschke I agree on that. But the problem I had is that when `fill_value` was not specified, and `NaN` values were used, then the order does matter, because:
```
>>> max(25, float('NaN'))
25
>>> max(float('NaN'), 25)
nan
```
Which is weird, and seems to me like a bug in Python.
Gotcha, sorry I assume the premise was if fill_value was not `None`.

FWIW, `np.max` is at least consistent:

```
In [9]: np.max((float('nan'), 25))
Out[9]: nan

In [10]: np.max((25, float('nan')))
Out[10]: nan
```
thanks @brute4s99 
Yay \o/