Skip to content
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

MAINT: Simplify return value of ndimage functions. #8461

Merged
merged 3 commits into from
Mar 5, 2018

Conversation

jaimefrio
Copy link
Member

Simplifies the return value of ndimage functions, so that the output array is always returned, instead of the awkward current behavior, where many (but not all) functions only return the output array if it has been allocated by the function, and return None if it has been passed by the user through the output= keyword argument.

@alan-isaac
Copy link

I am not making an objection but rather a offering a comment to document the apparent but so far unmentioned motivation for the original behavior: the idea that a function that mutates its argument should return None. It is therefore hard to consider such behavior unexpected or unintuitive; it might even be considered to be preferred. (E.g., https://mail.python.org/pipermail/python-dev/2003-October/038855.html )

@jaimefrio
Copy link
Member Author

I understand and respect the distinction between .sort and sorted. And if we had two different functions, one taking an output and returning nothing, one not taking it and returning a new array, I could be convinced that's a reasonable state of things.

What I find harmful is having a single function that behaves wildly different depending on what its inputs are, because it basically forces you to have something like:

if output is None:
    return _nd_image_function(...)
else:
    _nd_image_function(...)
    return output

instead of the much more simple, readable and understandable:

return _nd_image_function(...)

@jaimefrio
Copy link
Member Author

Aside from the change in return value behaviour, this also fixes a bug in the return value of binary_erosion with brute_force=True, iterations=0 and an output array was given: if the first iteration of the erosion returned the input unchanged, the result was never stored in output. Have added a test for that case.

@rgommers rgommers added scipy.ndimage maintenance Items related to regular maintenance tasks labels Mar 2, 2018
@rgommers
Copy link
Member

rgommers commented Mar 2, 2018

Needs a rebase. On merge also needs an entry in https://github.com/scipy/scipy/wiki/Release-note-entries-for-SciPy-1.1.0

@rgommers
Copy link
Member

rgommers commented Mar 2, 2018

+1 for this change

Jaime Fernandez and others added 3 commits March 2, 2018 21:29
THe original code was broken: if we called binary_erosion with iterations=0 and
brute_force=True, and the first iteration returned the input unchanged, the output
array wasn't being modified at all, and was being returned as initialized.
@jaimefrio
Copy link
Member Author

Have rebased. Once this is merged, will add the following text to the release notes in the Backwards incompatible changes section.

Functions in the ndimage module now always return their output array. Before this most functions only returned the output array if it had been allocated by the function, and would return None if it had been provided by the user.

@rgommers rgommers added this to the 1.1.0 milestone Mar 5, 2018
@rgommers rgommers merged commit ed61f17 into scipy:master Mar 5, 2018
@rgommers
Copy link
Member

rgommers commented Mar 5, 2018

Okay in it goes. Thanks @jaimefrio!

@jaimefrio jaimefrio deleted the ndimage_return branch March 5, 2018 19:40
@jaimefrio
Copy link
Member Author

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.ndimage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants