Skip to content

BUG: fix numpy min/max compat with index (#26125) #26324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 9, 2019

Conversation

Batalex
Copy link
Contributor

@Batalex Batalex commented May 9, 2019

When working on the original issue, I came across something similar :

  • RangeIndex had its own implementation of max/min with the same behavior
  • Index & RangeIndex could both benefit from adding another compat call
In [5]: np.argmin(pd.Index([1,2,3]), out=0)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
venv\lib\site-packages\numpy\core\fromnumeric.py in _wrapfunc(obj, method, *args, **kwds)
     55     try:
---> 56         return getattr(obj, method)(*args, **kwds)
     57

TypeError: argmin() got an unexpected keyword argument 'out'

During handling of the above exception, another exception occurred:

TypeError                                 Traceback (most recent call last)
<ipython-input-5-5c9b894867ef> in <module>
----> 1 np.argmin(pd.Index([1,2,3]), out=0)

venv\lib\site-packages\numpy\core\fromnumeric.py in argmin(a, axis, out)
   1170
   1171     """
-> 1172     return _wrapfunc(a, 'argmin', axis=axis, out=out)
   1173
   1174

venv\lib\site-packages\numpy\core\fromnumeric.py in _wrapfunc(obj, method, *args, **kwds)
     64     # a downstream library like 'pandas'.
     65     except (AttributeError, TypeError):
---> 66         return _wrapit(obj, method, *args, **kwds)
     67
     68

venv\lib\site-packages\numpy\core\fromnumeric.py in _wrapit(obj, method, *args, **kwds)
     44     except AttributeError:
     45         wrap = None
---> 46     result = getattr(asarray(obj), method)(*args, **kwds)
     47     if wrap:
     48         if not isinstance(result, mu.ndarray):

TypeError: output must be an array

I changed it to be consistent with the rest of pandas ValueError: the 'out' parameter is not supported in the pandas implementation of argmax().

@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #26324 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26324      +/-   ##
==========================================
- Coverage   92.04%   92.03%   -0.01%     
==========================================
  Files         175      175              
  Lines       52291    52297       +6     
==========================================
+ Hits        48132    48134       +2     
- Misses       4159     4163       +4
Flag Coverage Δ
#multiple 90.59% <100%> (ø) ⬆️
#single 40.74% <50%> (-0.16%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/range.py 97.97% <100%> (+0.01%) ⬆️
pandas/core/base.py 98% <100%> (+0.01%) ⬆️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97.01% <0%> (-0.12%) ⬇️

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 d024aae...4f1eecd. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #26324 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26324      +/-   ##
==========================================
- Coverage   92.04%   92.03%   -0.01%     
==========================================
  Files         175      175              
  Lines       52291    52297       +6     
==========================================
+ Hits        48132    48134       +2     
- Misses       4159     4163       +4
Flag Coverage Δ
#multiple 90.59% <100%> (ø) ⬆️
#single 40.74% <50%> (-0.16%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/range.py 97.97% <100%> (+0.01%) ⬆️
pandas/core/base.py 98% <100%> (+0.01%) ⬆️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97.01% <0%> (-0.12%) ⬇️

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 d024aae...4f1eecd. Read the comment docs.

@jreback jreback added the Compat pandas objects compatability with Numpy or Python functions label May 9, 2019
@jreback jreback added this to the 0.25.0 milestone May 9, 2019
@jreback jreback merged commit 2bca899 into pandas-dev:master May 9, 2019
@jreback
Copy link
Contributor

jreback commented May 9, 2019

thanks @Batalex

@Batalex Batalex deleted the fix/numpy-max-index branch May 13, 2019 11:49
@TomAugspurger
Copy link
Contributor

I'm a bit surprised at the performance impact here http://pandas.pydata.org/speed/pandas/#index_object.Range.time_min (about 20-25%). It may be worth seeing if the performance of our numpy compat validators can be improved.

@TomAugspurger
Copy link
Contributor

On second thought, maybe the relative perf slowdown isn't too surprising, since RangeIndex.min / max is basically free. So it looks like a large slowdown, but in absolute terms it's not so bad... Still may be worth profiling things to see if there's any easy wins to be had in the validators (as in theory those should be cheap too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compat pandas objects compatability with Numpy or Python functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: np.min(pd.Index) and np.max(pd.Index) raise a TypeError
3 participants