Don't reset the fill_value of a MaskedArray when calling view() with no dtype - #2703

This change makes sense to me, the original code doesn't. It was done on purpose though. I would propose to also add a fill_value=None kw to view() and document that if not given and dtype is given, the fill value will be reset. Now that may still be a surprise. For example, if I had an int32 array and took an int64 view, I would not expect the fill value to be reset.

@rgommers - I did this. I agree it's confusing that an int64 view of an int32 array does not preserve the fill_value, but I guess it's hard to make it automatic - would a string view of an int32 array set the fill value to a string representation of the previous string value? One thing we could do is emit a warning if the fill value gets reset. What do you think?

I don't understand the two Travis failures - any ideas?

Thanks the changes to view look good. Some minor issues with the tests.

I think with clear documentation like you added here the situation is OK, adding a warning could be annoying too.

The Travis failures are unrelated, just ignore them.

@rgommers - thanks for the review, I'll fix the minor issues. Now regarding

x.view(MaskedArray)

and

x.view(np.ndarray)

this does seem to be a legitimate use of view. From ndarray.view:

``a.view()`` is used two different ways:

``a.view(some_dtype)`` or ``a.view(dtype=some_dtype)`` constructs a view
of the array's memory with a different data-type.  This can cause a
reinterpretation of the bytes of memory.

``a.view(ndarray_subclass)`` or ``a.view(type=ndarray_subclass)`` just
returns an instance of `ndarray_subclass` that looks at the same array
(same shape, dtype, etc.)  This does not cause a reinterpretation of the
memory.

so this is the first example of the second way. Technically, it should be type=ma.MaskedArray for example, but it seems that just setting a single argument works as type if dtype is a sub-class of ndarray.

I should add some tests explicitly specifying type=, but do you agree that it makes sense to leave

x.view(MaskedArray)
x.view(np.ndarray)

since this is acceptable behavior (for ndarray.view, and therefore by extension, MaskedArray.view)?

Hmm, never noticed that. Looks very ugly to me, and the ndarray.view documentation of the dtype parameter doesn't mention it at all (only further down this is mentioned).

So yes, then the test is OK. Documenting this better in both view() methods would be useful.

@rgommers - ok, will do - should I include an improvement of the np.ndarray.view docstring in this PR, or should I keep it separate?

Adding it to this PR works for me.

@rgommers - I've updated the docstrings, and added a few more tests. I also removed the print statements. Does it look ok now? (let me know if you have suggestions for tweaking the docstrings)

This looks good to me now. I'll leave it open for a day or two in case someone else wants to comment.

@rgommers - just wanted to check whether this can be merged? Thanks!

Yes, merged. Thanks @astrofrog