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

CLN: removed unused parameter 'out' from Series.idxmin/idxmax #12638

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@mortada
Contributor

mortada commented Mar 16, 2016

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

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 16, 2016

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.

codecov-io commented Mar 16, 2016

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.

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Mar 16, 2016

Contributor

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

Contributor

jreback commented Mar 16, 2016

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

@jreback jreback added the Compat label Mar 16, 2016

@jreback jreback added this to the 0.18.1 milestone Mar 16, 2016

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Mar 16, 2016

Contributor
Contributor

jreback commented Mar 16, 2016

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Mar 17, 2016

Contributor

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

Contributor

jreback commented Mar 17, 2016

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

@mortada

This comment has been minimized.

Show comment
Hide comment
@mortada

mortada Mar 18, 2016

Contributor

@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

return argmin(axis, out)

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?

Contributor

mortada commented Mar 18, 2016

@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

return argmin(axis, out)

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?

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Mar 18, 2016

Member

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

Member

gfyoung commented Mar 18, 2016

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

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Mar 18, 2016

Contributor

@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

Contributor

jreback commented Mar 18, 2016

@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

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Mar 18, 2016

Member

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

Member

gfyoung commented Mar 18, 2016

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

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Mar 18, 2016

Member

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?

Member

gfyoung commented Mar 18, 2016

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?

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Mar 18, 2016

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

Contributor

jreback commented Mar 18, 2016

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)

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Mar 18, 2016

Member

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

Member

gfyoung commented Mar 18, 2016

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

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Mar 18, 2016

Contributor

@gfyoung yeh something that that would work

Contributor

jreback commented Mar 18, 2016

@gfyoung yeh something that that would work

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Apr 6, 2016

Member

@jreback , @mortada : this PR is being subsumed by my massive PR #12810.

Member

gfyoung commented Apr 6, 2016

@jreback , @mortada : this PR is being subsumed by my massive PR #12810.

@mortada

This comment has been minimized.

Show comment
Hide comment
@mortada

mortada Apr 7, 2016

Contributor

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

Contributor

mortada commented Apr 7, 2016

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

@mortada mortada closed this Apr 7, 2016

@jreback jreback removed this from the 0.18.1 milestone Apr 7, 2016

@jreback jreback added this to the 0.18.1 milestone Apr 30, 2016

gfyoung added a commit to gfyoung/pandas that referenced this pull request May 1, 2016

COMPAT: Expand compatibility with fromnumeric.py
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 added a commit that referenced this pull request May 1, 2016

COMPAT: Expand compatibility with fromnumeric.py
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

nps added a commit to nps/pandas that referenced this pull request May 17, 2016

COMPAT: Expand compatibility with fromnumeric.py
Closes pandas-dev#12638
Closes pandas-dev#12644
Closes pandas-dev#12687

Author: gfyoung <gfyoung17@gmail.com>

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

429bc51 [gfyoung] COMPAT: Expand compatibility with fromnumeric.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment