Ensure copy preserves masked arrays #3852
Conversation
This to preserve masking upon deepcopy refs dask#3848
| assert_eq(x, xx) | ||
|
|
||
| assert_eq(y, t) | ||
| assert isinstance(y.compute(), np.ma.masked_array) |
There was a problem hiding this comment.
Are type checks not already part of assert_eq? If not, then maybe we should add that generally. It's nice to apply checks to assert_eq because then they get applied everywhere.
|
cc @pp-mo for review |
|
@mrocklin: I am sure this is not a part of `assert_eq` as I was somewhat
confused why my first rendition of my test did not work. I can have a look
sometime next week if I can elevate this check to `assert_eq` as you
suggest, but be warned, I might get back to you with questions as I am not
very familiar with writing tests ;)
…On Sun, Aug 5, 2018, 16:25 Matthew Rocklin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dask/array/tests/test_masked.py
<#3852 (comment)>:
> @@ -28,6 +29,23 @@ def test_from_array_masked_array():
assert_eq(dm, m)
+def test_copy_deepcopy():
+ t = np.ma.masked_array([1, 2], mask=[0, 1])
+ x = da.from_array(t, chunks=t.shape, asarray=False)
+ #x = da.arange(5, chunks=(2,))
+ y = x.copy()
+ memo = {}
+ y2 = deepcopy(x, memo=memo)
+
+ xx = da.ma.masked_where([False, True], [1,2])
+ assert_eq(x, xx)
+
+ assert_eq(y, t)
+ assert isinstance(y.compute(), np.ma.masked_array)
Are type checks not already part of assert_eq? If not, then maybe we
should add that generally. It's nice to apply checks to assert_eq because
then they get applied everywhere.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3852 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGgExA2mYr3JY-_8CsNbLH8a6wQp0RYmks5uNwBNgaJpZM4VvYLb>
.
|
|
@mrocklin, cc @jcrist as he wrote all of This actually has a function But then we get into the trouble of defining when two arrays are equal: this check passes everywhere, except in Should we add a kwarg to I'd personally propose to keep this test specific to |
|
One other issue with asserting types blindly in I don't remember why I made a Personally, I'd go with the smallest change to get this in for now, but if you want to try expanding |
| assert_eq(dm, m) | ||
|
|
||
|
|
||
| def test_copy_deepcopy(): |
There was a problem hiding this comment.
This is fine, but I also might remove this test from ma, and expand the test_copy_mutate test in dask_array_core.py to test an array of another dtype. That feels more like the correct place, as its testing the copy method, which isn't ma specific.
There was a problem hiding this comment.
@mrocklin: holiday without proper internet does not do wonders to my productivity. I will try to have a look this week as I think this is now straightforward, but my schedule is looking somewhat busy.
There was a problem hiding this comment.
From @jcrist 's tone on this it seems like he's ok either way. I think it's also ok to keep this in this testing file mostly as a regression test. It is testing ma-specific functionality just to ensure that they stay masked arrays.
|
Merging this at the end of the workday if there are no further comments. |
|
Yes, let's just merge it like this if nobody has further comments, although I think there could be some more cleanup and hardening in |
|
Thanks for the fix @TAdeJong . Merging! |
|
Thanks for following up @pp-mo |
flake8 daskFixes #3848, at least for the presented case, with the fix suggested by @jcrist.
I added a test to array/masked to test for this as well. I checked that this test fails for the old situation and passes for the new one, but I do not have the overview whether it is in the right location or even right format. Feedback is very much appreciated.