-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
BUG: MA ufuncs should set mask to False, not array([False]) #7350
Conversation
Scalar/array scalar |
Right, this PR changes it so the second example gives just |
@@ -2968,8 +2968,8 @@ def __array_wrap__(self, obj, context=None): | |||
np.copyto(result, fill_value, where=d) | |||
# Update the mask | |||
if m is nomask: | |||
if d is not nomask: | |||
m = d | |||
if any(d): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up calling MaskedArray.any()
; maybe cleaner to just write d.any()
?
More generally, for the common case of largish arrays with no domain problems, the whole domain now gets checked twice. Might it be an idea to combine with the copyto
and the whole finding of fill_value
? I.e.,
if d.any():
try:
... get fill_value ...
np.copyto(result, fill_value, where=d)
if m is nomask:
m = True if d.all() else d
else:
m = (m | d)
(Haven't thought deeply enough to be sure whether or not one could more the result = result.copy()
inside the if-statement as well.)
It is good to look at things separately. See comment about possible speed-up. |
if d is not nomask: | ||
m = d | ||
if any(d): | ||
m = True if all(d) else d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this part necessary? Or is it OK to set the mask to a full array of True
? It is not tested. (also, write as d.all()
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt it is OK to set the mask of a full array of True to True. At the very least it would break assignments, since they check nomask/False explicitly but not allmasked or whatever you would call it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I've been rethinking these lines. The original point of this PR was to turn an array of False into False.
But for an array of True taken care of here, I probably don't want to turn it into just True. It's possible I want to leave it as an array of True, but masked
is also a possibility.
I'm also investigating the original problem that this PR is meant to fix (it was making it difficult to reimplement MaskedArray.var
) to see if there is a better way to fix that altogether.
be4ef91
to
db2a5d6
Compare
OK, I went for an even more drastic change: I removed the whole section of code which tries to update the out-of-domain data.
I updated with @mhvk's comments about using Appveyor seems down. Tests pass on my system in python 2 though. |
I am actually a bit confused about this right now with "masked" and "nomask" and all, I guess masked is only a singleton, and never The branch can't be merged, you will have to do a rebase. |
6f2f97d
to
e6fb300
Compare
Rebased on master, checks pass now. @seberg, yes, perhaps technically this isn't a bug. But I think this PR makes more sense, and also it is a performance/sanity boost for all the cases throughout the maskedarray code where there is a test of the form This PR makes my updated It this looks good I will squash the last commit, we can wait for tests to run, and then I will address your comments in #5706 :) |
I've never been enamored of these attempts to keep values in place when masked, so in that sense am happy with removing all that code. It is a bit of a change in the way it works, though... Otherwise, this looks all OK. |
I think there might be people who use masked arrays as a way to say "I want On Fri, Mar 4, 2016 at 5:08 PM, Marten van Kerkwijk <
Nathaniel J. Smith -- https://vorpus.org http://vorpus.org |
e6fb300
to
fb747df
Compare
@njsmith , I think that's what's described in the MaskedArray docs:
The behavior promised is actually tested, in test_datafriendly_div and related tests. My guess is the lines I removed are related. It's not exactly the same though - the promise above is to (sometimes) avoid modifying masked elements, while the lines I removed place the I actually don't see much point in promising to try to leave masked elements alone sometimes, but then in the next line telling users that promise is unreliable. I would rather remove such code and make no such promises. But this PR doesn't even go that far, since I am only removing different undocumented behavior. So, I still favor removing the code. The behavior related to leaving masked values alone is unaffected. |
@ahaldane - ah, yes, you're right, this is even more specific than trying to leave values in place. I now see no reason not to remove the lines. |
Pinging this one. If we merge it, then I can rebase #5706 and continue work over there. |
@ahaldane - sorry to go back on what I wrote before, but my sense would be, after all, to keep the filling lines. The reason is that I think we should either remove it altogether (i.e., not even construct the |
fb747df
to
37d3019
Compare
That's fair, I updated it along those lines. |
OK, as far as I can see, that makes it entirely uncontroversial. |
Let's give it a shot. Thanks Allan. I note that #5706 has already been merged. |
Thanks Chuck |
Example of the problem:
(This PR is split off from #5706. It fixes a problem that turned up in later commits there)