Skip to content

Remove 'out' from docstrings for wrapped numpy unary functions #2117

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

Closed
wants to merge 1 commit into from

Conversation

dcherian
Copy link
Contributor

Inserting the string "The out keyword argument is not supported" is hacky. Can we do without that and just remove out=None from the signature?

@fmaussion
Copy link
Member

Thanks!

I wonder is this is the best way to do this but I don't have a better idea either. Surely @shoyer can weight in.

Anyways, this doesn't pass through the tests because sometimes __doc__ is None. Also I think we need some kind of test for this.

@dcherian
Copy link
Contributor Author

Ugh, serves me right for submitting PRs late at night. Thanks, @fmaussion. I'll wait for feedback on the approach before fixing it.

@shoyer
Copy link
Member

shoyer commented May 12, 2018

The cleaner solution here would be to try to get rid of _method_wrapper by writing explicit methods/functions implemented with apply_ufunc:

# methods which pass on the numpy return value unchanged
# be careful not to list methods that we would want to wrap later
NUMPY_SAME_METHODS = ['item', 'searchsorted']
# methods which don't modify the data shape, so the result should still be
# wrapped in an Variable/DataArray
NUMPY_UNARY_METHODS = ['astype', 'argsort', 'clip', 'conj', 'conjugate']
PANDAS_UNARY_FUNCTIONS = ['isnull', 'notnull']

This is the strategy suggested in this TODO note:

TODO(shoyer): rewrite this module, making use of xarray.core.computation,
NumPy's __array_ufunc__ and mixin classes instead of the unintuitive "inject"
functions.

This is a bit more verbose but it would be much more obvious what's going on.

clip would probably be the place to start, since it's the only one with an invalid out argument. You could just copy the relevant parts of the docstring from NumPy.

@dcherian
Copy link
Contributor Author

OK It'll take me some time to get to that. Closing.

@dcherian dcherian closed this May 14, 2018
@dcherian dcherian deleted the fix-docstring branch August 15, 2019 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants