Repository: pydata/xarray

Issue:
Comparing scalar xarray with ma.masked fails with ValueError: assignment destination is read-only
#### Code Sample, a copy-pastable example if possible

```python
xarray.DataArray(0) > numpy.ma.masked
```
#### Problem description

This results in `ValueError: assignment destination is read-only`:

```
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-102-f6226708b971> in <module>()
----> 1 xarray.DataArray(0) > numpy.ma.masked

/group_workspaces/cems2/fiduceo/Users/gholl/anaconda3/envs/FCDR37a/lib/python3.7/site-packages/xarray/core/dataarray.py in func(self, other)
   1808
   1809             variable = (f(self.variable, other_variable)
-> 1810                         if not reflexive
   1811                         else f(other_variable, self.variable))
   1812             coords = self.coords._merge_raw(other_coords)

/group_workspaces/cems2/fiduceo/Users/gholl/anaconda3/envs/FCDR37a/lib/python3.7/site-packages/xarray/core/variable.py in func(self, other)
   1580                             if not reflexive
   1581                             else f(other_data, self_data))
-> 1582             result = Variable(dims, new_data)
   1583             return result
   1584         return func

/group_workspaces/cems2/fiduceo/Users/gholl/anaconda3/envs/FCDR37a/lib/python3.7/site-packages/xarray/core/variable.py in __init__(self, dims, data, attrs, encoding, fastpath)
    260             unrecognized encoding items.
    261         """
--> 262         self._data = as_compatible_data(data, fastpath=fastpath)
    263         self._dims = self._parse_dimensions(dims)
    264         self._attrs = None

/group_workspaces/cems2/fiduceo/Users/gholl/anaconda3/envs/FCDR37a/lib/python3.7/site-packages/xarray/core/variable.py in as_compatible_data(data, fastpath)
    177             dtype, fill_value = dtypes.maybe_promote(data.dtype)
    178             data = np.asarray(data, dtype=dtype)
--> 179             data[mask] = fill_value
    180         else:
    181             data = np.asarray(data)

ValueError: assignment destination is read-only
```

#### Expected Output

To be consistent, the result should be identical to the result of

```
(xarray.DataArray([0,0]) > numpy.ma.masked)[0]
```

which would be

```
xarray.DataArray(nan) 
```

#### Output of ``xr.show_versions()``

<details>
xarray.show_versions()

INSTALLED VERSIONS
------------------
commit: None
python: 3.7.0.final.0
python-bits: 64
OS: Linux
OS-release: 2.6.32-754.el6.x86_64
machine: x86_64
processor: x86_64
byteorder: little
LC_ALL: None
LANG: en_GB.UTF-8
LOCALE: en_GB.UTF-8

xarray: 0.10.7
pandas: 0.23.2
numpy: 1.14.5
scipy: 1.1.0
netCDF4: 1.4.0
h5netcdf: 0.6.1
h5py: 2.8.0
Nio: None
zarr: None
bottleneck: 1.2.1
cyordereddict: None
dask: 0.18.1
distributed: 1.22.0
matplotlib: 2.2.2
cartopy: 0.16.0
seaborn: 0.9.0
setuptools: 39.2.0
pip: 18.0
conda: None
pytest: 3.2.2
IPython: 6.4.0
sphinx: None
</details>


Discussion:
welcome[bot]: 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](http://docs.xarray.dev/en/stable/contributing.html).

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](https://github.com/Unidata/netcdf4-python/) 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 in 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 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 https://github.com/pydata/xarray/issues/2377#issuecomment-415074188.

There @shoyer suggested to just use:

```python
data = duck_array_ops.where_method(data, ~mask, fill_value)
```

instead of 
```python
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 ;)

Patch:
diff --git a/doc/whats-new.rst b/doc/whats-new.rst
index 40766ed8..3d661b30 100644
--- a/doc/whats-new.rst
+++ b/doc/whats-new.rst
@@ -27,6 +27,8 @@ New Features
 - Add support for lshift and rshift binary operators (``<<``, ``>>``) on
   :py:class:`xr.DataArray` of type :py:class:`int` (:issue:`7727` , :pull:`7741`).
   By `Alan Brammer <https://github.com/abrammer>`_.
+- Fix `as_compatible_data` for masked float arrays, now always creates a copy when mask is present (:issue:`2377`, :pull:`7788`).
+  By `Max Hollmann <https://github.com/maxhollmann>`_.
 
 
 Breaking changes
diff --git a/xarray/core/variable.py b/xarray/core/variable.py
index c19cb21c..b4026ed3 100644
--- a/xarray/core/variable.py
+++ b/xarray/core/variable.py
@@ -274,8 +274,7 @@ def as_compatible_data(data, fastpath=False):
         mask = np.ma.getmaskarray(data)
         if mask.any():
             dtype, fill_value = dtypes.maybe_promote(data.dtype)
-            data = np.asarray(data, dtype=dtype)
-            data[mask] = fill_value
+            data = duck_array_ops.where_method(data, ~mask, fill_value)
         else:
             data = np.asarray(data)
 


Test patch:
diff --git a/xarray/tests/test_variable.py b/xarray/tests/test_variable.py
index bef5efc1..a6dffb82 100644
--- a/xarray/tests/test_variable.py
+++ b/xarray/tests/test_variable.py
@@ -2560,6 +2560,19 @@ class TestAsCompatibleData:
         assert_array_equal(expected, actual)
         assert np.dtype(float) == actual.dtype
 
+        original = np.ma.MaskedArray([1.0, 2.0], mask=[True, False])
+        original.flags.writeable = False
+        expected = [np.nan, 2.0]
+        actual = as_compatible_data(original)
+        assert_array_equal(expected, actual)
+        assert np.dtype(float) == actual.dtype
+
+        # GH2377
+        actual = Variable(dims=tuple(), data=np.ma.masked)
+        expected = Variable(dims=tuple(), data=np.nan)
+        assert_array_equal(expected, actual)
+        assert actual.dtype == expected.dtype
+
     @pytest.mark.filterwarnings("ignore:Converting non-nanosecond")
     def test_datetime(self):
         expected = np.datetime64("2000-01-01")