pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

closes #xxxx
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

It seems misleading to have the out parameter in the function signature when it is not used:

pd.Series.argmin(self, axis=None, out=None, skipna=True)
pd.Series.argmax(self, axis=None, out=None, skipna=True)

(unlike the numpy.argmin/argmax methods where out can be used)

Current coverage is `88.64%`

Merging #12638 into master will not affect coverage as of 4bff739

@@ master #12638 diff @@

Files 275 275
Stmts 133688 133688
Branches 0 0
Methods 0 0

Hit 118511 118512 +1 Partial 0 0
Missed 15177 15176 -1

Review entire Coverage Diff as of 4bff739

Powered by Codecov. Updated on successful CI builds.

this is the same issue as being fixed in #12603. The underlying PR is almost there, so need to recast this similary.

jreback added the Compat

pandas objects compatability with Numpy or Python functions

label

Mar 16, 2016

now that #12603 is merged, this should be quite trivial.

@jreback one problem is that numpy actually passes both axis and out as positional arguments
https://github.com/numpy/numpy/blob/master/numpy/core/fromnumeric.py#L1036

so it seems like we have two choices:

keep the current function signature but make sure out is None, i.e.

def idxmin(self, axis=None, out=None, skipna=True): if out is not None: raise ValueError(msg)

change it to

def idxmin(self, axis=None, *args, skipna=True): validate_args(args, min_length=0, max_length=1)

which would be better?

@jreback : Is there any reason why we're supporting an axis argument when there aren't any "axes" to a Series object, not to mention, it's not even used in the implementation?

@mortada : Your second option doesn't work because you can't put keywords after *args, but a more customized solution might be needed given that the keyword we want (skipna) comes after the keyword(s) that we don't want. A suitable solution I think depends on how many keywords we can remove from the signature.

@mortada can you give a concrete example of where this actually fails now?

we should hide the out param, but since numpy is passing it positionally, prob have to interpret if its passed as the axis param.

@gfyoung it makes the signatures the same for the same methods in Series/DataFrame. there is consistency across methods in pandas, that's why most are already in generic.py. Series only methods still have the axis param again for consistency.

E.g. s.sum(0) makes sense, so why not s.idxmin(0)

In fact these routines idxmin/idxmax, ideally we could consolidate them and put them in generic.py

@jreback : By passing in arguments like this, idxmin will interpret numpy's out parameter as its skipna parameter. It used to be that skipna would just default to True in these cases.

Since this issue is localized to Series, could we just create a method that validates that skipna is in the tuple (None, True, False) and assign it to be True if skipna is None?

Signature: pd.DataFrame.idxmin(self, axis=0, skipna=True)
Signature: pd.Series.idxmin(self, axis=None, out=None, skipna=True)

I would like to make the signture of Series.idxmin/max the same as DataFrame.idxmin/max w/o causing issues. IF its passing by position, then this is pretty easy. (just interpret skipna), are there cases where its NOT passed by position (but by kwargs)? if so, may need to add a kwargs handler as well (like we did for stat functions)

@jreback : On the numpy side in fromnumeric.py, it internally is by position and not keyword (again, why that is the case, I'm not sure). In that case, we could go with handling skipna as I proposed above.

@gfyoung yeh something that that would work

@gfyoung makes sense, I'll close this one, thanks!

gfyoung added a commit to forking-repos/pandas that referenced this pull request

May 1, 2016

Augment pandas array-like methods with appropriate parameters (generally, '*args' and '**kwargs') so that they can be called via analogous functions in the numpy library they are defined in 'fromnumeric.py'.

Closes pandas-devgh-12638. Closes pandas-devgh-12644. Closes pandas-devgh-12687.

jreback pushed a commit that referenced this pull request

May 1, 2016

Closes #12638 Closes #12644 Closes #12687

Author: gfyoung gfyoung17@gmail.com

Closes #12810 from gfyoung/fromnumeric-compat and squashes the following commits:

429bc51 [gfyoung] COMPAT: Expand compatibility with fromnumeric.py

Labels

Compat