Changing MaskedArray.squeeze() to never return masked

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Changing MaskedArray.squeeze() to never return masked

Eric Wieser

When using ndarray.squeeze, a view is returned, which means you can do the follow (somewhat-contrived) operation:

>>> def fill_contrived(a):
        a.squeeze()[...] = 2
        return a

>>> fill_contrived(np.array([1]))
array(2)

However, when tried with a masked array, this can fail, breaking liskov subsitution:

>>> fill_contrived(np.ma.array([1], mask=[True]))
MaskError: Cannot alter the masked element.

This fails because squeeze breaks the contract of returning a view, instead deciding sometimes to return masked.

There is a patch that fixes this in gh-9432 - however, by necessity it breaks any existing code that uses m_arr.squeeze() is np.ma.masked.

Is this too breaking a change?

Eric


_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Changing MaskedArray.squeeze() to never return masked

Benjamin Root
This sort of change seems very similar to the np.diag() change a few years ago. Are there lessons we could learn from then that we could apply to here?

Why would the returned view not be a masked array?

Ben Root

On Tue, Jul 18, 2017 at 9:37 AM, Eric Wieser <[hidden email]> wrote:

When using ndarray.squeeze, a view is returned, which means you can do the follow (somewhat-contrived) operation:

>>> def fill_contrived(a):
        a.squeeze()[...] = 2
        return a

>>> fill_contrived(np.array([1]))
array(2)

However, when tried with a masked array, this can fail, breaking liskov subsitution:

>>> fill_contrived(np.ma.array([1], mask=[True]))
MaskError: Cannot alter the masked element.

This fails because squeeze breaks the contract of returning a view, instead deciding sometimes to return masked.

There is a patch that fixes this in gh-9432 - however, by necessity it breaks any existing code that uses m_arr.squeeze() is np.ma.masked.

Is this too breaking a change?

Eric


_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion



_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Changing MaskedArray.squeeze() to never return masked

Allan Haldane
On 07/18/2017 09:52 AM, Benjamin Root wrote:
> This sort of change seems very similar to the np.diag() change a few years
> ago. Are there lessons we could learn from then that we could apply to here?
>
> Why would the returned view not be a masked array?
>
> Ben Root

I am in favor of the proposed change below.

I'd like to merge it, but before that I want to make sure I understand
your comment.

Are you referring to the proposed change to make diag return a view
instead of a copy? Note that this has not actually happened yet:
https://github.com/numpy/numpy/issues/7661

Also, I think this case is different because it does not change core
numpy, rather this is to make the MaskedArray module act more
consistently with core numpy. Because of that I think it is much less
problematic than the diag changes.

Cheers,
Allan


> On Tue, Jul 18, 2017 at 9:37 AM, Eric Wieser <[hidden email]>
> wrote:
>
>> When using ndarray.squeeze, a view is returned, which means you can do
>> the follow (somewhat-contrived) operation:
>>
>>>>> def fill_contrived(a):
>>         a.squeeze()[...] = 2
>>         return a
>>>>> fill_contrived(np.array([1]))
>> array(2)
>>
>> However, when tried with a masked array, this can fail, breaking liskov
>> subsitution:
>>
>>>>> fill_contrived(np.ma.array([1], mask=[True]))
>> MaskError: Cannot alter the masked element.
>>
>> This fails because squeeze breaks the contract of returning a view,
>> instead deciding sometimes to return masked.
>>
>> There is a patch that fixes this in gh-9432
>> <https://github.com/numpy/numpy/pull/9432> - however, by necessity it
>> breaks any existing code that uses m_arr.squeeze() is np.ma.masked.
>>
>> Is this too breaking a change?
>>
>> Eric
>> ​
>>
>> _______________________________________________
>> NumPy-Discussion mailing list
>> [hidden email]
>> https://mail.python.org/mailman/listinfo/numpy-discussion
>>
>>
>
>
>
> _______________________________________________
> NumPy-Discussion mailing list
> [hidden email]
> https://mail.python.org/mailman/listinfo/numpy-discussion
>

_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Changing MaskedArray.squeeze() to never return masked

Benjamin Root
Yes, that is the change I am thinking of. And yes, it hasn't happened yet. But, it has been set to warn for a few years now, and there was a lot of controversy over it when it was first proposed. That said, I do think the way it was handled made sense, and it is a good model to follow for these types of changes.

For all intents and purposes, MaskedArray is "core numpy" for many users. Yes, it has its quirks, but it has been very stable for many years, and users have gotten used to the quirks. While I am all for taking steps to eliminate as many quirks as possible, we need to be mindful of such potentially disruptive changes and give users enough of a heads-up about it.

Ben Root




On Thu, Aug 10, 2017 at 1:00 PM, Allan Haldane <[hidden email]> wrote:
On 07/18/2017 09:52 AM, Benjamin Root wrote:
> This sort of change seems very similar to the np.diag() change a few years
> ago. Are there lessons we could learn from then that we could apply to here?
>
> Why would the returned view not be a masked array?
>
> Ben Root

I am in favor of the proposed change below.

I'd like to merge it, but before that I want to make sure I understand
your comment.

Are you referring to the proposed change to make diag return a view
instead of a copy? Note that this has not actually happened yet:
https://github.com/numpy/numpy/issues/7661

Also, I think this case is different because it does not change core
numpy, rather this is to make the MaskedArray module act more
consistently with core numpy. Because of that I think it is much less
problematic than the diag changes.

Cheers,
Allan


> On Tue, Jul 18, 2017 at 9:37 AM, Eric Wieser <[hidden email]>
> wrote:
>
>> When using ndarray.squeeze, a view is returned, which means you can do
>> the follow (somewhat-contrived) operation:
>>
>>>>> def fill_contrived(a):
>>         a.squeeze()[...] = 2
>>         return a
>>>>> fill_contrived(np.array([1]))
>> array(2)
>>
>> However, when tried with a masked array, this can fail, breaking liskov
>> subsitution:
>>
>>>>> fill_contrived(np.ma.array([1], mask=[True]))
>> MaskError: Cannot alter the masked element.
>>
>> This fails because squeeze breaks the contract of returning a view,
>> instead deciding sometimes to return masked.
>>
>> There is a patch that fixes this in gh-9432
>> <https://github.com/numpy/numpy/pull/9432> - however, by necessity it
>> breaks any existing code that uses m_arr.squeeze() is np.ma.masked.
>>
>> Is this too breaking a change?
>>
>> Eric
>> ​
>>
>> _______________________________________________
>> NumPy-Discussion mailing list
>> [hidden email]
>> https://mail.python.org/mailman/listinfo/numpy-discussion
>>
>>
>
>
>
> _______________________________________________
> NumPy-Discussion mailing list
> [hidden email]
> https://mail.python.org/mailman/listinfo/numpy-discussion
>

_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion


_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Loading...