Overall:

• the release notes text in this PR needs some fixing, but so does the text that's currently in 1.11, I think.
• the warning logic here seems clearer than that in master -- I'm not sure what oldsharedmask is about, and with this PR I don't have to figure it out :-)
• agreed that the refcnt check is klugey and optional. But I guess if it's a temporary measure that reliably increases the specificity of the warning messages then I'm +0.5.
• the changes to extras.py seem obviously beneficial (not sure how we were getting away without these before? Are these functions not tested?)

So modulo comments above my impression is that something like this should probably be merged.

Cc @jakirkham - thoughts?

@njsmith we are "getting away" with it, because our test suit is broken when it comes to warning testing, especially FutureWarnings, see my suppressed warnings PR ;).

Bah, one other annoying thing, though maybe we can ignore it. The mask is of course never a view if it is nomask.

So, I haven't had lots of time to take a close look. Though my impression after looking through is this looks very similar to the previous version ( #7187 ). My criticisms are pretty much the same.

Not a fan of reference counting as this could result in weird behavior in terms of performance and validity (of warning). Concerned as this will not raise warnings when there are views of views (and so on). This is what _oldsharedmask was for @njsmith. There seem to be some functions that are tweaking internal state _sharedmask, which seems at best risky. Though those could probably be easily fixed by calling unshare_mask instead. Finally, it looks like a change is being added to the covariance/correlation computation, which seems orthogonal to the warning (feel free to correct me). Maybe that is a separate desirable change, but it should probably be discussed first and independently of this change.

Though the big question for me right now is whether this change is needed. I haven't heard any complaints about the warning level on the initial issue. Though that doesn't mean that there are no complaints. So, my question is, are there still problems downstream? Are there problems here? If so, what are they?

Bah, one other annoying thing, though maybe we can ignore it. The mask is of course never a view if it is nomask.

This appears to already be addressed here thanks to the fact that the first branch rules that out ( https://github.com/numpy/numpy/pull/7363/files#diff-e85f023d9c07df014a190b485db72c3dR3229 ).

@jakirkham, it annoys me because it is inconsistent in the final behaviour. Your desired end result is, that mask changes tend to propagate back to the original array, but they won't in the case of nomask.

As to the rest, as I said, the reference counting is only a further filter, and yes, it is safe even for views of views, because that is what it checks. That only "self" holds a reference to the mask and the mask is not a view. If someone had a view into that same mask, they would also -- indirectly -- have a reference to it. Feel free to proof me wrong, but I predict unless you use wekrefs you won't find a loophole. You are right about bad code smell, but I will just claim my nose is better trained.

If you check carefully, all the tweaks in _sharedmask are for temporary arrays, and they are necessary (though probably not with the refcount in place) since they cause warnings that users cannot avoid, e.g. [1]. As for the removal of the unshare_mask calls, they are obviously not needed, since the mask is replaced in the following line, and all unshare mask does is replace the mask and set _sharedmask to False.

About _oldsharedmask, if you really believe it is necessary, could you please explain if it can possibly do more then keep warning for the same masked array? That part I can see, but I don't really see it as necessary (and I am not even sure I think it is not worse).

[1]:

# have import warnings; warnings.simplefilter("always") in place.
In [43]: m = np.ma.masked_greater(np.random.random((10, 10)), 0.5)
In [44]: np.ma.median(m, axis=0)
/home/sebastian/.local/lib/python2.7/site-packages/numpy/ma/core.py:3195: MaskedArrayFutureWarning: Currently, slicing will try to return a view of the data, but will return a copy of the mask. In the future, it will try to  return both as views. This means that using `__setitem__` will propagate values back through all masks that are present.
  MaskedArrayFutureWarning
Out[44]: 
masked_array(...)

In [45]: np.ma.corrcoef(m)
/home/sebastian/.local/lib/python2.7/site-packages/numpy/ma/core.py:3195: MaskedArrayFutureWarning: Currently, slicing will try to return a view of the data, but will return a copy of the mask. In the future, it will try to  return both as views. This means that using `__setitem__` will propagate values back through all masks that are present.
  MaskedArrayFutureWarning
+ extra warnings.
Out[45]: 
masked_array(...)

In [46]:  np.nanmedian([[np.nan], [np.nan]], axis=1)
/home/sebastian/.local/lib/python2.7/site-packages/numpy/ma/core.py:3195: MaskedArrayFutureWarning: Currently, slicing will try to return a view of the data, but will return a copy of the mask. In the future, it will try to  return both as views. This means that using `__setitem__` will propagate values back through all masks that are present.
  MaskedArrayFutureWarning
/home/sebastian/.local/lib/python2.7/site-packages/numpy/lib/nanfunctions.py:769: RuntimeWarning: All-NaN slice encountered
  warnings.warn("All-NaN slice encountered", RuntimeWarning)
/home/sebastian/.local/lib/python2.7/site-packages/numpy/lib/nanfunctions.py:769: RuntimeWarning: All-NaN slice encountered
  warnings.warn("All-NaN slice encountered", RuntimeWarning)
Out[46]: array([ nan,  nan])

it annoys me because it is inconsistent in the final behaviour. Your desired end result is, that mask changes tend to propagate back to the original array, but they won't in the case of nomask.

I thought you were musing over quieting the warning by a check for nomask. You are right. @mhvk raised this as well and IIRC suggested that we look into getting rid of that next.

As to the rest, as I said, the reference counting is only a further filter...

Yeah, still feel like ref counting is a bit sketchy. Not sure how much we can trust this given how masked arrays are constructed.

If you check carefully, all the tweaks in _sharedmask are for temporary arrays, and they are necessary (though probably not with the refcount in place) since they cause warnings that users cannot avoid...

Again haven't had lots of time to take a close look. It seems like a call to unshare_mask would have the same effect here and be less sketchy though. This is just my gut reaction.

About _oldsharedmask, if you really believe it is necessary, could you please explain if it can possibly do more then keep warning for the same masked array

When I have more time I'll try to give a better explanation. To say it simply and roughly, it tries to mimic what the future behavior will be. I suppose it could be tweaked more...

Though when these improvements were contemplated the question was raised about how accurate we really want to make the warning and whether more accurate might not be worse by lulling people into reliance on a nearly (not entirely) accurate warning. This brings me to my original question, which I am still not seeing the answer to. What problems are we still solving? If it is to silence a few false alarms in covariance, median, etc., maybe we should just silence those by following the recommend procedure from before namely calling unshared_mask in those places.

What to do about this? Looking at Pauli's tests I see two occurances of MaskedArrayFutureWarning, one each in MatPlotLib and StatsModels, which doesn't seem much. Whether they are valid warnings, I don't know, but they vanish in comparison to the other warnings. @njsmith The _oldsharedmask thing is pretty straight forward once you figure out what is going on, which admittedly took me a while. A bit more explanation in the PR would probably have helped there. All in all, I'm inclined to leave things as they are, although I have some niggling doubts as to whether we really want to make the change.

There is mention of other independent parts of this PR that look desireable. Could someone expand on that?

Sorry, I was trying to be clever using _oldsharedmask, but it does make things a bit difficult to follow at first pass.

As for as the proposal presented here, below are my thoughts. Others may of course think differently.

Seem like good ideas:

• The release notes write up seems nice.
• The update warning text could be included.
• The stacklevel flag for the warning could be added.

Subject of discussion:

• Movement of the warning into the branch.

Seem like not good ideas:

• Reference counting for warning determination.
• Workarounds using "private" member variables.
• Suggested use of "private" member variables.

Movement of the warning into the branch.

My impression is that the main effect of this is to put the warning directly onto the same code path that is actually going to be changed in the future, which is usually the simplest and most accurate way to do things. Is that wrong? Can you elaborate on what the actual trade-offs are here?

Reference counting for warning determination.

It seems safe and accurate to me, but meh, whatever, I'm not so excited that I want to argue about it :-)

Workarounds using "private" member variables.
Suggested use of "private" member variables.

IIUC, what you're saying here is that you don't like the recommendation that users do marr._sharedmask = False if they want to silence the warning by opting-in to the future behavior early. (a) do I in fact understand correctly? (b) if I do, then do you have some other suggestion for how to do this? It's indeed unfortunate that it's an underscore member variable, but sometimes these things happen, and AFAIK it is safe and works correctly for all past and future versions of numpy, which is an extremely valuable property...

Hi,
I and a colleague were scratching our heads to try to get a mask shared with a masked array, so this upcoming change is a great news ! I just have one remark about this sentence in the changelog:

If you want to get the future behavior early, use masked_view._sharedmask = False

At first when reading this PR I thought we would have to wait for 1.12, and this sentence was not clear because of the meaning of "_sharedmask": setting it to False seems like it prevent the mask to be shared, though it is actually the contrary. So now that I read the code I understand, but maybe the sentence should be more explicit ?
Also it seems that this workaround can be compatible with at least 1.10 ?

If anyone wants me to do explicit fixup stuff here (i.e. remove the
refcounting check or whatever), I will do it.
My last argument: I have gh-7099, and posted the numbers here. I feel
it is easier to add this here and putting in <10 (ok, wihtout
refcounting check it was quite a bit worse) suppressions into our tests
than putting in almost 70. Could try to figure out module wide
suppressions, but I am not sure it works reliably. Plus it helps
others.

The general point whether we want it. Dunno, in general I think I
prefer the behaviour, but there are also inconsistencies with MAs that
do not have anything masked, so I would prefer if actual users like
@ahaldane spoke up.

I've been meaning to review this but just haven't had time! I'll take a look once I get a chance.

(by the way I wouldn't consider myself a MaskedArray user really... I got interested in MaskedArrays due to changes I wanted to make elsewhere in numpy, which snowballed)

I'm going to stick with what we have for 1.11.0. If there are internal uses that raise warnings, we should deal with that at some point, but the number of warnings currently generated downstream don't strike me as excessive. I will try to update the release notes, but with some editing as I find the version here confusing, but otherwise an improvement as it offers a more extended explanation.

Updated release notes in #7430, comments welcome.

@seberg @jakirkham I'm thinking we might solve the nomask problem by making the _mask attribute a property and using an object that may, or may not, contain an actual mask. If no mask is contained the property getter will return nomask, otherwise an actual mask. Since a reference to that object can be retained in a view, the setter can set the mask in the view and it will show up in the original. As the maxim goes, "All problems in computer science can be solved by another level of indirection".

EDIT, We might want to control access with a lock, or maybe not, we don't bother to do that with ordinary views and I don't know if array assignments or property getters/setters are atomic.

Hehe, yeah but another level of indirection would make it even harder
to maintain ;). I am sure if you fixed up the confusing stuff in the
current text, it is good.

Ceterum censeo, that moving the place to warn has only advantages,
and those silencing _sharedmask = False things are "obviously correct"
(i.e. on temporaries) and really should be there.
But I really don't want to delay the release over this, though I always
hoped that you guys would rethink and simply agree....
But "obvious" in the eye of the beholder I guess ;).

I went and checked both Matplotlib and StatsModels for both versions and this PR does a better job. So merging. Thanks Sebastian.

I and a colleague were scratching our heads to try to get a mask shared with a masked array, so this upcoming change is a great news !

@saimn, that warms my heart. 😄

@charris, ok, if you think it is better, I trust your judgement.

In particular, the StatsModel warning went away, which was good because it didn't seem associated with any test. For Matplotlib, the warning was in a different place and looked valid whereas the warning without this PR did not look correct. Not sure what the difference was there. Setting the stacklevel also helped.

@jakirkham I figured the only valid way to settle the question was to gather some actual data and it does look like this PR performs better.

EDIT: I was hoping they would both work the same, but it didn't turn out that way.

No need to keep selling it. 😄 I have learned my lesson after the disaster that was my first attempt at this warning. 😉

A little late here since you just merged, but I was just looking through the history of changes here and this PR looks ok to me.

I was at first a little worried about the refcounting, but @seberg's comment that this is merely a further filter on the warning satisfied me: There are no cases where we should emit a warning but don't (it can't happen that the mask is really shared but appears unshared by refounting), although there are still some rare cases where we probably don't need to warn but do (eg if the users manually makes a reference to arr._mask, but doesn't write to it).

@seberg Just to be sure, assignment to a masked data value can also change the mask, does this cover that case?

@charris sorry, a bit daft here, can you give an example? My logic here always was: Put the warning where the future change is and trust that it must be right.

@SeberT Actually, looks like a miss...

In [1]: import warnings

In [2]: warnings.simplefilter('always')

In [3]: x = ma.array(eye(2), mask=1)

In [4]: x
Out[4]: 
masked_array(data =
 [[-- --]
 [-- --]],
             mask =
 [[ True  True]
 [ True  True]],
       fill_value = 1e+20)

In [5]: x[0,0] = 1

In [6]: x
Out[6]: 
masked_array(data =
 [[1.0 --]
 [-- --]],
             mask =
 [[False  True]
 [ True  True]],
       fill_value = 1e+20)

No warning issued.

NVM, bad example...

EDIT: Looks OK.

Heh, almost difficult to get a good example:

In [1]: mask = np.ones((2, 2), dtype=bool)

In [2]: import warnings; warnings.simplefilter("always")

In [3]: x = np.ma.MaskedArray(np.eye(2), mask)

In [4]: x[0, 0] = 1
runtests.py:1: MaskedArrayFutureWarning: setting an item on a masked array which has a shared mask will not copy the mask and also change the original mask array in the future.
Check the NumPy 1.11 release notes for more information.
  #!/usr/bin/env python