Skip to content

MAINT: remove wrapper functions from numpy.core.multiarray #12470

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 4 commits into from
Dec 2, 2018

Conversation

shoyer
Copy link
Member

@shoyer shoyer commented Dec 1, 2018

The original motivation for the style of these wrapper functions, introduced
in gh-12175, was to preserve introspection. But it turns out NumPy's functions
defined in C don't support introspection anyways, so the extra wrapper
functions are entirely pointless.

This version reverts the additional wrapper functions, which put default
arguments in two places and introduced slow-down due to the overhead of
another function call.

I've retained docstrings in multiarray.py, since it's definitely more readable
to keep docstrings and dispatchers together rather than leaving docstrings in
_add_newdocs.py.

One bonus of this approach is that dispatcher functions have the same name
as their implementations, so np.concatenate(unknown=True) gives an
error message mentioning "concatenate" rather than "_concatenate_dispatcher":
TypeError: concatenate() got an unexpected keyword argument 'unknown'

The original motivation for the style of these wrapper functions, introduced
in numpygh-12175, was to preserve introspection. But it turns out NumPy's functions
defined in C don't support introspection anyways, so the extra wrapper
functions are entirely pointless.

This version reverts the additional wrapper functions, which put default
arguments in two places and introduced slow-down due to the overhead of
another function call.

I've retained docstrings in multiarray.py, since it's definitely more readable
to keep docstrings and dispatchers together rather than leaving docstrings in
_add_newdocs.py.

One bonus of this approach is that dispatcher functions have the same name
as their implementations, so `np.concatenate(unknown=True)` gives an
error message mentioning "concatenate" rather than "_concatenate_dispatcher":
`TypeError: concatenate() got an unexpected keyword argument 'unknown'`
@ahaldane
Copy link
Member

ahaldane commented Dec 1, 2018

Might the C functions ever support introspection? That is, is it possible to set __signature__ attribute on C methods? I vaguely thought it was, last time I looked at it a year ago, it's just a bit tricky. This SO answer suggests it can be done.

@ahaldane
Copy link
Member

ahaldane commented Dec 1, 2018

Actually, I think that last time it was discussed, it wasn't feasible to remove the signatures from the docstrings because __signature__ only worked in python 3. But since we are dropping python2 soon, then there is no more obstacle to adding signatures to all the C methods.

@shoyer
Copy link
Member Author

shoyer commented Dec 1, 2018

@ahaldane The good part about this current approach is that if we ever do add signatures on C functions, they'll get copied over onto the generated dispatched functions.

To be honest, I'm not sure what I was thinking for adding this extra layers of indirection at all.

@@ -1158,6 +1158,27 @@ arr_unravel_index(PyObject *self, PyObject *args, PyObject *kwds)

char *kwlist[] = {"indices", "shape", "order", NULL};

/* TODO: remove this in favor of warning raised in the dispatcher when
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style for multiline comments is

/*
 * blah
 * blah
 */

@charris charris merged commit dc608ba into numpy:master Dec 2, 2018
@charris
Copy link
Member

charris commented Dec 2, 2018

Thanks Stephan.

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

Successfully merging this pull request may close these issues.

3 participants