ERR: stat function kwarg interpretation #12301

Closed
mikeyshulman opened this Issue Feb 11, 2016 · 13 comments

Comments

Projects
None yet
3 participants

df.max(axi=1) does not throw an error and evaluates to df.max()

Contributor

jreback commented Feb 12, 2016

these functions accept **kwargs in order to avoid us having to name certain numpy arguments (e.g. out), which are not generally acceptable to pandas. I suppose that we could check if any but certain args are there and raise a nice error message.

As I don't want to have out=None in the signature nor the doc-string.

pull-requests would be welcome

jreback changed the title from max kwarg interpretation to stat function kwarg interpretation Feb 12, 2016

jreback changed the title from stat function kwarg interpretation to ERR: stat function kwarg interpretation Feb 12, 2016

jreback added this to the Next Major Release milestone Feb 12, 2016

Member

gfyoung commented Feb 13, 2016

I'm not entirely sure how all of these functions are set up, but when I tried taking the **kwargs argument out of just one of the method signatures here, it breaks a ton of tests (about 40). I keep on getting TypeError: stat_func() got an unexpected keyword argument 'out', which trace back to function calls in fromnumeric.py in numpy such as amax(axis=axis, out=out)

IINM it seems that this tolerance for "invalid" arguments is quite ingrained into the codebase, so much so that even the tests seem to allow it.

Member

gfyoung commented Feb 13, 2016

Unless someone can explain otherwise, it seems that the best that can be done is to check whether kwargs is non-empty, after which you can raise a warning like: UserWarning: invalid arguments passed into {insert function name}. Please pass in only arguments listed in the function signature

Contributor

jreback commented Feb 13, 2016

what u can do is pop out from other kwargs
then if it's not empty raise

iirc out is the only arg that we accept that's not listed (and we don't want it) but allow for compat

Member

gfyoung commented Feb 13, 2016

How about just accepting any arguments that currently break the tests? out is not the only one. dtype is another parameter that breaks at least one test.

Contributor

jreback commented Feb 13, 2016

yeah that too

Contributor

jreback commented Feb 13, 2016

the reason iirc I did this in the first place because of the API changes in various versions of numpy and didnt want to have to deal with strict checking in case something was added

Contributor

jreback commented Feb 13, 2016

I am ok with fine grained checking
I just don't really want them in the signature

Member

gfyoung commented Feb 13, 2016

That's fair. It's too bad that there are so many naming conflicts between the numpy and pandas codebase, for the **kwargs is not even used in the functions, only for compatibility between the libraries. Should **kwargs be included in documentation? I did not realize that this was the reasoning initially.

Contributor

jreback commented Feb 13, 2016

not sure what u mean by naming conflicts

Member

gfyoung commented Feb 13, 2016

What I meant by "conflict" was that the trace back always kept originating to function calls in the numpy library that evidently must have also been defined in pandas IIUC.

Contributor

jreback commented Feb 13, 2016

btw we first try to dispatch to bottleneck (if installed), then numpy. things are tested with both.

@jreback jreback modified the milestone: 0.18.0, Next Major Release Feb 13, 2016

@gfyoung gfyoung added a commit to gfyoung/pandas that referenced this issue Feb 14, 2016

@gfyoung gfyoung BUG: Prevent abuse of kwargs in stat functions
Filters kwargs argument in stat functions to
prevent the passage of clearly invalid arguments
while at the same time maintaining compatibility
with analogous numpy functions.

Closes gh-12301.
f9de80f

@jreback jreback added a commit that referenced this issue Feb 14, 2016

@gfyoung @jreback gfyoung + jreback BUG: Prevent abuse of kwargs in stat functions
Addresses issue #12301 by filtering `kwargs` argument in stat
functions to prevent the passage of clearly invalid arguments while at
the same time maintaining compatibility with analogous `numpy`
functions.

Author: gfyoung <gfyoung17@gmail.com>

Closes #12318 from gfyoung/kwarg_remover and squashes the following commits:

f9de80f [gfyoung] BUG: Prevent abuse of kwargs in stat functions
3d2f115
Contributor

jreback commented Feb 14, 2016

closed by #12318

jreback closed this Feb 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment