BUG: Series.argmax() fails with np.inf
State: closed

closes #13595

This PR changes the behavior of two functions in the `nanops` module to address #13595, `nanargmax` and `nanargmin`. Previously, these two functions used a flag on the `_get_values` helper that caused it to 1) always mask out and ignore infinite values in the input, and 2) attempt to coerce the input to float.

It's worth noting that because of 2), this changes the way `nanargmax` and `nanargmin`, and consequently `Series.idxmax` etc, behave with string data. Previously,

```python
data = ['foo', 'bar', 'baz']

pd.Series(data).idxmax() # --> ValueError

pd.DataFrame({'A': data}).idxmax() # --> ValueError

pd.DataFrame({'grp': [1, 1, 2], 'A': data}).groupby('grp').idxmax() # --> an empty data frame
```

After the PR,

```python
In [1]: import pandas as pd

In [2]: data = ['foo', 'bar', 'baz']

In [3]: pd.Series(data).idxmax()
Out[3]: 0

In [4]: pd.DataFrame({'A': data}).idxmax()
Out[4]:
A    0
dtype: int64

In [5]: pd.DataFrame({'grp': [1, 1, 2], 'A': data}).groupby('grp').idxmax()
Out[5]:
     A
grp
1    0
2    2
```

This is consistent with the behavior of `numpy.argmax` etc with string data, and might be more consistent with user expectations. It did mean that I needed to update one of the `DataFrame.groupby` test cases, but otherwise appears to cause no breakage.

Since this might be a bigger change that was expected for fixing the issue, I'm posting it as WIP.

 - [x] closes #13595
 - [x] tests added / passed
 - [x] passes ``git diff upstream/master --name-only -- '*.py' | flake8 --diff``
 - [x] whatsnew entry


Labels:
name: Bug
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=h1) Report
> Merging [#16449](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0f25426eac5c0097214df78b45125ac039c85770?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/16449/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #16449      +/-   ##
==========================================
- Coverage   91.03%   90.98%   -0.05%     
==========================================
  Files         162      162              
  Lines       49527    49529       +2     
==========================================
- Hits        45086    45065      -21     
- Misses       4441     4464      +23
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.76% <100%> (-0.03%)` | :arrow_down: |
| #single | `40.25% <50%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/nanops.py](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbmFub3BzLnB5) | `97.65% <100%> (-0.39%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `63.23% <0%> (-1.82%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/16449?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.72% <0%> (-0.1%)` | :arrow_down: |

------

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

Test cases are failing on Python 2.7, because with `s = pd.Series(['spam', np.nan])`, to compute argmin/max we ultimately end up calling NumPy argmin/max on an array with `object` dtype, which just compares by value. In Python 2.7, strings can be compared to both None and `np.nan` and we get a useful result. In Python 3, strings cannot be compared to None or nan, and we get a `TypeError`. I currently have the `TypeError` as the expected behavior in the test cases. We could

- run different tests for Py 2 vs Py 3 and have test cases for expected behavior under both (seems bad), or
- make some further updates to the `nanops` module to gracefully handle Series with strings and missing data. In this case, `s.argmax(skipna=False)` would reproduce the Python 2 behavior, and `s.argmax()` would give the expected answer ignoring NAs.

Some quick fiddling around leads me to think that this might be possible by adding some special values to the `nanops` module that always compare as less than everything else, and using these as the mask values in `_get_values` when the dtype is `object`. Along the lines of

```python
class Smallest():
    def __lt__(self, other):
        return True
    def __gt__(self, other):
        return False
```

Or, possibly simpler, for `object` Series, mask all NA values (`None` or `np.nan`) with `np.nan`, and then call down to `np.nanargmax`.
its ok on the object types in py2. you can simply have an if/else which tests separately on py2 (check for raise) and py3 (valid). it would be nice to have this work with nans but its a can of worms I think. you can certainly make it work in certain situations, but you would find that it quickly gets out of control (meaning if the user has mixes of numeric/strings it simply doesn't work). but this is the same thing numpy does.

There *is* a way around it.

```
In [1]: from pandas.core.algorithms import safe_sort

In [2]: arr = np.array(['a', np.nan, 'b'])

In [4]: arr.argmax()
Out[4]: 1

In [5]: safe_sort(arr, np.arange(len(arr)))
Out[5]: 
(array(['a', 'b', 'nan'], 
       dtype='<U3'), array([0, 2, 1]))
```
1st value is sorted, 2nd is argsort

prob slower, but will work (so you would only do this for ``object`` on py2).


> it would be nice to have this work with nans but its a can of worms I think. 

I'll stick with the simple approach, then.
Okay, the CI builds are reporting errors from the test suite, and I'm having trouble reproducing these errors locally. In CircleCI it looks like the error is coming from `pandas/tests/test_expressions.py`, but when I run `pytest pandas/tests/test_expressions.py` locally the tests pass fine. I do have `numexpr` installed, so I think the tests are actually being run instead of skipped. The same thing happens under Python 2 and Python 3. Am I missing something obvious? How can I get more information about the environment that CircleCI is using?

```
± pytest pandas/tests/test_expressions.py
========================= test session starts =========================
platform darwin -- Python 3.6.1, pytest-3.1.2, py-1.4.34, pluggy-0.4.0
rootdir: /Users/dgrady/Documents/pandas, inifile: setup.cfg
plugins: cov-2.3.1
collected 20 items

pandas/tests/test_expressions.py ....................

====================== 20 passed in 1.64 seconds ======================
```
you can look at circle.yml these run under various locales
Thanks — looks like this failure is caused by upgrading from NumPy 1.12.x to NumPy 1.13, and that it's fixed in upstream master. I think I could fix it by rebasing the PR branch onto master, and force-pushing back to my fork of the Pandas repo. Is that the right etiquette for this project?
yes u always should master and force push if needed
Responded to your "Changes Requested" above; is this good to go?
got a bit lost. pls rebase / update
How does this look now?
Pinging on green
thanks @DGrady nice changes!
Thanks for all your feedback, and patience @jreback !