issue_comments

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

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

@dcherian Sure, done :)

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

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

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

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

@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 Do you need any adjustments to merge this?

@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?

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

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

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

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?

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

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.