8000 DOC: Fix documentation on MaskedArray.filled by gfyoung · Pull Request #6920 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

DOC: Fix documentation on MaskedArray.filled #6920

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 4, 2016
Merged

DOC: Fix documentation on MaskedArray.filled #6920

merged 1 commit into from
Jan 4, 2016

Conversation

gfyoung
Copy link
Contributor
@gfyoung gfyoung commented Jan 1, 2016

Addresses documentation issue in #6647 in which an alias to self was being returned instead of a copy when there were no masked values to fill.

@jaimefrio
Copy link
Member

I don't use masked arrays at all, but not always getting a copy, and this depending on the values of the input seems a recipe for 8000 disaster. I think it makes more sense to make the function comply with the documented behavior than vice versa. It seems to be just a matter of adding a .copy() to the proper return.

@gfyoung
Copy link
Contributor Author
gfyoung commented Jan 1, 2016

Fair enough. The issue was classified under Documentation, which is why I went that route. However, thinking about it, I do see what you mean. I'll change the doc back and just return copies.

@gfyoung gfyoung changed the title DOC: Fix documentation on MaskedArray.filled BUG: Always Return a Copy for MaskedArray.filled Jan 2, 2016
@gfyoung
Copy link
Contributor Author
gfyoung commented Jan 2, 2016

Getting test failures due to erroneous self in my own test, as well as another test failure in which there is an assertion statement regarding the memory addresses that now fails because we are returning a copy at all times. Should not be an issue to fix.

@charris
Copy link
Member
charris commented Jan 2, 2016

I think we looked at this before. Changing the return type can cause backward compatibility problems, as you saw with the failing test.

@gfyoung
Copy link
Contributor Author
gfyoung commented Jan 2, 2016

@charris : Question, should we migrate towards returning a copy for everything? I do see merit in what @jaimefrio is suggesting. If so, I could mention that in the documentation.

@gfyoung gfyoung changed the title BUG: Always Return a Copy for MaskedArray.filled DOC: Fix documentation on MaskedArray.filled Jan 2, 2016
@gfyoung
Copy link
Contributor Author
gfyoung commented Jan 2, 2016

Reverted changes back to a purely documentation fix. Besides my question about migrating towards returning a copy for everything ( @charris ), I think it should be good now to merge.

charris added a commit that referenced this pull request Jan 4, 2016
@charris
Merge pull request #6920 from gfyoung/filled_documentation
2fb2b66 
DOC: Fix documentation on MaskedArray.filled
@charris charris merged commit 2fb2b66 into numpy:master Jan 4, 2016
@charris
Copy link
Member
charris commented Jan 4, 2016

Thanks @gfyoung .

@gfyoung gfyoung deleted the filled_documentation branch January 4, 2016 22:05
@ben-e-whitney ben-e-whitney mentioned this pull request Sep 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component: documentation component: numpy.ma masked arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gfyoung @jaimefrio @charris
0