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

[DGrady]

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,

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,

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.

• closes Series.argmax() fails with np.inf #13595
• tests added / passed
• passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
• whatsnew entry

[jreback]

pls add a whatsnew note for 0.21.0. You can put in bug fixes.

[jreback]

this line is too long for the linter

[jreback]

can you add another tests which asserts the behavior for strings

[DGrady]

I think this is covered now, starting on line 1898 below

[jreback]

can you add the case for an empty series as well.

[jreback]

if you do

with pd.option_context('mode.use_inf_as_null', True):
    ...

I suspect this will fail. Can you test with this as well.
(you may have to get the value of the option and pass to isfinite=pd.get_option('mode.use_inf_as_null'))

[DGrady]

Ah, I didn't even know about this! So based on the notes on the options documentation page, it looks like Pandas used to treat floating point +/- inf, as well as nan, as missing values by default, and this behavior was changed at some point. That's a big help; will check this and update.

[DGrady]

@jreback can you confirm this is the expected behavior?

s = pd.Series([0, -np.inf, np.inf, np.nan])

with pd.option_context('mode.use_inf_as_null', True):
    s.argmax() # --> 0
    s.argmax(skipna=False) # --> nan

[DGrady]

This looks like it's working; see line 1891 below

[codecov]

Codecov Report

Merging #16449 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            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%)	⬇️
#single	40.25% <50%> (-0.07%)	⬇️

Impacted Files	Coverage Δ
pandas/core/nanops.py	97.65% <100%> (-0.39%)	⬇️
pandas/io/gbq.py	25% <0%> (-58.34%)	⬇️
pandas/plotting/_converter.py	63.23% <0%> (-1.82%)	⬇️
pandas/core/frame.py	97.72% <0%> (-0.1%)	⬇️

---

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 0f25426...9a3d62d. Read the comment docs.

[DGrady]

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

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.

[jreback]

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).

[DGrady]

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.

[jreback]

this is very confusing. pls re-word.

[jreback]

use compat.PY3

[jreback]

I would actually be ok with NOT allowing this for strings at all. IOW you can use the @disallow decorator. It is a slight break with numpy, but it doesn't work now anyhow. I am not sure why allowing this is a good thing when it has different py2/3 behavior (and it doesn't work on mixed things).

[DGrady]

Not sure what you mean by “it doesn't work now anyhow”? I think the build failures were just a linting error that I missed locally; trying again.

I think the argument for allowing it with strings is consistency: right now, the Pandas version of argmax etc is inconsistent with base Python, with NumPy, and with other Pandas functions. For example,

data = ['spam', 'eggs']
data_a = np.array(data, dtype='object')
data_s = pd.Series(data)

max(data) # -> 'spam'
data_a.max() # -> 'spam'
data_s.max() # -> 'spam'

# So far so good

data.index(max(data)) # -> 0
data_a.argmax() # -> 0
data_s.argmax() #-> ValueError: could not convert string to float: 'eggs'

This is confusing; we expect data_s.argmax() to return 0 just like everything else. After this PR, it does return 0, as expected.

It's true that if we change the example above to use mixed data types, like data2 = ['spam', 'eggs', None], then the behavior is different under Python 2 and Python 3, but I think that even in this case this PR makes the Pandas behavior more consistent with the prevailing behavior of whatever version of Python you're using. For example,

data2 = ['spam', 'eggs', None]
data2_a = np.array(data2, dtype='object')
data2_s = pd.Series(data2)

max(data2) # -> 'spam' under Py2, TypeError under Py3
data2_a.max() # -> 'spam' under Py2, TypeError under Py3
data2_s.max() # -> 'spam' under Py2, TypeError under Py3

data2.index(max(data2)) # -> 0 under Py2, TypeError under Py3
data2_a.argmax() # -> 0 under Py2, TypeError under Py3

Now, with the current release of Pandas, data2_s.argmax() will throw a ValueError under Py2 and Py3 with a message about converting strings to floats. After this PR, data2_s.argmax() will return 0 under Py2, as we would expect, and will throw a TypeError with a message about incomparable types under Py3.

[jreback]

I would be ok with allowing this, but it HAS to work with missing values. Everything else does and no reason for this not too. you simply mask, apply function (e.g. argmax), then replace the missing by -1

[jreback]

I think it better to put in the fix for inf, then in a followup fix the strings.

[DGrady]

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 ======================

[jreback]

you can look at circle.yml these run under various locales

[DGrady]

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?

[jreback]

yes u always should master and force push if needed

[DGrady]

Responded to your "Changes Requested" above; is this good to go?

[jreback]

use double-backticks around the function names argmin, as well for Series and DataFrame. you can say with groupby as GroupBy is not an external term.

[jreback]

I would be ok with allowing this, but it HAS to work with missing values. Everything else does and no reason for this not too. you simply mask, apply function (e.g. argmax), then replace the missing by -1

[jreback]

I think it better to put in the fix for inf, then in a followup fix the strings.

[jreback]

got a bit lost. pls rebase / update

[jreback]

I think just a minor change and should be good to go. ping on green and i'll review again.

[jreback]

these are changed slightly, e.g. use mode.use_inf_as_na

[DGrady]

Done

[jreback]

actually we need to have a mini-section in api breaking changes that min/max now don't work on object dtypes.

and if you can add a couple of tests (does any exiting ones break because of the disallow('O') changes?

[DGrady]

Okay; I wrote something for the breaking API changes section. Ultimately, though, the difference is just that argmax used to throw ValueError and now throws TypeError with string data; is that an appropriate change for the breaking section?

I added a couple of other tests for Series with object dtype. I haven't seen any existing tests break as a result of this change.

[DGrady]

How does this look now?

[jreback]

oh thought this worked in some cases before? If its just a type of error change, then just do a 1-liner, but move to API breaking section (and you need the issue reference number)

[DGrady]

Sounds good. Right, I think what happened is:

• before the PR, argmin would raise ValueError with strings and objects
• while working on the PR, the fix in the nanops module allowed these functions to work with strings and objects, but only if there were no missing values. I updated some test cases to reflect this behavior.
• Our discussion led to deciding that this behavior is too inconsistent, that fixing it in general would be pretty complicated, so the best thing for now is to explicitly disallow nanargmin with object dtypes.
• so now, argmin raises TypeError with strings and objects.

[jreback]

put these error ones in a separate test function, use parametrize on it to avoid the boilerplate.

[DGrady]

Pinging on green

[jreback]

thanks @DGrady nice changes!

[DGrady]

Thanks for all your feedback, and patience @jreback !