Skip to content

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

Merged
merged 4 commits into from
Apr 2, 2015
Merged

BUG: Implemented MaskedArray.dot #5709

merged 4 commits into from
Apr 2, 2015

Conversation

abalkin
Copy link
Contributor

@abalkin abalkin commented Mar 23, 2015

MaskedArray used to inherit ndarray.dot which ignored masks in the operands.

Fixes issue #5185.

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)
Copy link
Member

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!

Copy link
Contributor Author

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).

@ahaldane
Copy link
Member

Otherwise looks good to me.

@charris
Copy link
Member

charris commented Mar 25, 2015

Be good to get this finished up.

@abalkin
Copy link
Contributor Author

abalkin commented Mar 26, 2015

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)
Copy link
Member

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).

@charris
Copy link
Member

charris commented Apr 1, 2015

Shouldn't this be the same as np.ma.extras.dot? Note that the latter has a strict keyword, but no out. What if out is not an instance of masked array, should that not raise an error explicitly instead of implicitly?

@abalkin
Copy link
Contributor Author

abalkin commented Apr 1, 2015

Shouldn't this be the same as np.ma.extras.dot?

No. MaskedArray methods should have the same signature as ndarrays. From the documentation:

The numpy.ma module provides a nearly work-alike replacement for numpy that supports data arrays with masks.

The current sorry state of affairs in np.ma.extras is not the reason to perpetuate the violations of the substitution principle.

@charris
Copy link
Member

charris commented Apr 1, 2015

Then I'd procede by first adding out to ma.dot and calling it from the method with strict=True. I note that currently the method behavior is different than the strict=False default of the function. Both options for strict could be useful, do you have any thoughts as to how to do that? Or do you think the non-strict version is a bad idea.

@abalkin
Copy link
Contributor Author

abalkin commented Apr 1, 2015

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 strict flag is just developer saying: I have no clue what a dot product of two masked arrays should be, so I'll give two versions hoping that one will do what user wants.

@abalkin
Copy link
Contributor Author

abalkin commented Apr 1, 2015

@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 @ operator is implemented and we will probably deprecate dot methods altogether, so there is no reason to try to make it perfect now.

@matthew-brett
Copy link
Contributor

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?

@charris
Copy link
Member

charris commented Apr 1, 2015

@abalkin But the mask returned here looks to propagate the strict=True logic. Am I missing something?

@abalkin
Copy link
Contributor Author

abalkin commented Apr 1, 2015

No, in my implementation ma.extra.dot is a call to MaskedArray.dot() after some mask manipulation done if strict=True. So x.dot(y) is the same as ma.dot(x, y, strict=False). This is proven by the fact that ma.dot tests pass unchanged.

@charris
Copy link
Member

charris commented Apr 1, 2015

You are right, my bad. But why not just call ma.dot with strict=False. After adding an out argument, of course. That way both method and function will be fixed/updated.

@charris
Copy link
Member

charris commented Apr 1, 2015

Hmm, OK, I missed that ma.dot now called the method. Why not add an out to the function also?

charris added a commit that referenced this pull request Apr 2, 2015
BUG: Implemented MaskedArray.dot
@charris charris merged commit 8b6effa into numpy:master Apr 2, 2015
@charris
Copy link
Member

charris commented Apr 2, 2015

Other fixups can be done later, so merging. @abalkin Could you add a note to 1.10-notes.rst mentioning the change in the dot method behavior? Probably in two places, under Compatibility notes for using the mask, and under New features for the out argument.

@abalkin
Copy link
Contributor Author

abalkin commented Apr 2, 2015

@charris - the out argument is not a new feature. The inherited method already had the out argument, it just did not produce correct results. This PR is strictly a bug fix, not a feature.

@charris
Copy link
Member

charris commented Apr 2, 2015

Point taken. I still think it needs a compatibility mention as the result has changed.

@abalkin
Copy link
Contributor Author

abalkin commented Apr 2, 2015

Yes, I'll try to come up with a short compatibility note.

@charris
Copy link
Member

charris commented Nov 3, 2015

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 dot function and then used in the method, not the other way round. In any case, if there is no activity to fix this up, I'm going to revert it.

@jjhelmus
Copy link
Contributor

jjhelmus commented Nov 3, 2015

I'll take a look at this tonight as see what can be done to improve the code and fix the regression.

@charris
Copy link
Member

charris commented Nov 3, 2015

Great, thanks.

@charris
Copy link
Member

charris commented Nov 5, 2015

@jjhelmus Any progress. This is the last thing that needs fixing before 1.10.2rc1.

@jjhelmus
Copy link
Contributor

jjhelmus commented Nov 6, 2015

Sorry have not had the time to properly look at this. Why don't we revert this for 1.10.2rc1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants