-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: Implemented MaskedArray.dot #5709
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
Conversation
MaskedArray used to inherit ndarray.dot which ignored masks in the operands. Fixes issue numpy#5185.
if out is None: | ||
d = np.dot(filled(self, 0), filled(other, 0)) | ||
m = ~np.dot(am, bm) | ||
return masked_array(d, mask=m) |
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.
Looking at the other ma functions, we want to preserve the input's type - at least one person I found cares, in #3907. With insipration fron other funcs, maybe do:
d = np.dot(filled(self, 0), filled(other, 0)).view(type(self))
d.__setmask__(~np.dot(am, bm))
return d
The calculation of the mask is cute, btw!
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 don't think using type(self) is correct here. It should be get_masked_subclass(self, other).
Otherwise looks good to me. |
Be good to get this finished up. |
I probably won't get to this until the weekend. I would like to add tests for the out ≠ None case and find a way to avoid calling getmaskarray if a mask is nomask. |
fx = mx.filled(0) | ||
r = mx.dot(mx) | ||
assert_almost_equal(r.filled(0), fx.dot(fx)) | ||
self.assertIs(r.mask, nomask) |
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.
assertIs
is not available in Python 2.6. Just use assert_(r.mask is nomask)
.
Shouldn't this be the same as |
No. MaskedArray methods should have the same signature as ndarrays. From the documentation:
The current sorry state of affairs in np.ma.extras is not the reason to perpetuate the violations of the substitution principle. |
Then I'd procede by first adding |
I think both strict=False and strict=True are wrong. See my note on the issue. I am strongly against any differences in ndarray and masked arrays APIs. The original promise of MA was to be a drop-in replacement for numpy. We should try to keep that promise. The |
@charris - I would like to start with fixing the bug and then discuss any enhancements. The current situation is that MaskedArray inherits ndarray's dot and appears working with the ndarray.dot's signature, but produces completely wrong results in presence of masked values. The dot behavior will certainly be revisited when |
But - it will be a very long time before ".dot" goes out of use - we would have to wait for Python 3.5 to be the oldest version that anyone is likely to use - say 10 years? |
@abalkin But the mask returned here looks to propagate the |
No, in my implementation |
You are right, my bad. But why not just call ma.dot with |
Hmm, OK, I missed that ma.dot now called the method. Why not add an out to the function also? |
BUG: Implemented MaskedArray.dot
Other fixups can be done later, so merging. @abalkin Could you add a note to |
@charris - the |
Point taken. I still think it needs a compatibility mention as the result has changed. |
Yes, I'll try to come up with a short compatibility note. |
There is a regression here, see #6611. Looking closely at this, I think the code could be made clearer. I also think the rework should be done in the |
I'll take a look at this tonight as see what can be done to improve the code and fix the regression. |
Great, thanks. |
@jjhelmus Any progress. This is the last thing that needs fixing before 1.10.2rc1. |
Sorry have not had the time to properly look at this. Why don't we revert this for 1.10.2rc1. |
MaskedArray used to inherit ndarray.dot which ignored masks in the operands.
Fixes issue #5185.