Fix as_compatible_data for read-only np.ma.MaskedArray

[maxhollmann]

• Closes Comparing scalar xarray with ma.masked fails with ValueError: assignment destination is read-only #2377
• Tests added
• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst

[welcome]

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

[kmuehlbauer]

I've marked this by accident, sorry @maxhollmann. Let us know when you feel this is ready

[maxhollmann]

Hi @kmuehlbauer, no worries! It's in draft because can't figure out how to reproduce this bug for the tests. data[mask] = fill_value was crashing when I tried to create a DataArray from a non-writeable MaskedArray I got via netCDF4 from a remote source. data = np.asarray(data, dtype=dtype) didn't set writeable to true in that case, but it does when I create a non-writeable MaskedArray in the tests. Any ideas how to test this properly?

[kmuehlbauer]

@maxhollmann I'll have a look into this, I think I've seen something like this some time ago.

Maybe you can add the tests to the PR or as comment? This might get more attention and will really help to debug.

[maxhollmann]

@kmuehlbauer Sure, I pushed the test as I was hoping it would work.

[kmuehlbauer]

@maxhollmann I've checked and memory served well, the following issue might be related: #2377. It looks like your use-case is at least connected to @gerritholl's. It would be great if you could add your original use case (as MCVE, if possible) to get more details.

A special case (masked integer arrays) is discussed in #3955. As this might give additional information, it might not exactly fit your problem.

[maxhollmann]

@kmuehlbauer For some reason I can't reproduce it anymore. I'll monitor whether it occurs again in the original situation and close this otherwise after some time.

[maxhollmann]

@kmuehlbauer Okay, I got it. It only seems to happen with float arrays. I adjusted the test, and it now fails without the fix.

Only tangentially related to this PR, but I noticed that as_compatible_data will modify the original data in this path, since asarray passes through the original and afterwards the masked values are replaced with fill_value. So masked_array > some_dataarray might modify masked_array. Shouldn't it default to creating a copy to prevent this?

[maxhollmann]

@kmuehlbauer Do you need any adjustments to merge this?

[kmuehlbauer]

@maxhollmann We might get at least some more views on this. There have been discussions on handling masked arrays and we should make sure this is exactly the solution we want to have.

@dcherian This changes as_compatible_data. Could you please have another look here? I'm a bit unclear about the implications.

[kmuehlbauer]

@maxhollmann I'm sorry, I'm still finding my way into Xarray. I've taken a closer look at #2377, especially #2377 (comment).

There @shoyer suggested to just use:

data = duck_array_ops.where_method(data, ~mask, fill_value)

instead of

data[mask] = fill_value

I've checked and it works nicely with your test. That way we would get away without the flags test and the special handling will take place in duck_array_ops. Would be great if someone can double check.

[dcherian]

I defer to @shoyer, the solution with where_method seems good to me.

[maxhollmann]

I like it, solves the concern in my previous comment as well. Updated the branch.

[dcherian]

Thanks @maxhollmann can you add a note to whats-new.rst please?

[maxhollmann]

@dcherian Sure, done :)

[dcherian]

Thanks @maxhollmann I pushed a test for #2377.

I see this is your first contribution to Xarray. Welcome! #1792 would be a nice contribution if you're looking for more to do ;)

[welcome]

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again!