Warn or immidiately change readonly flag on broadcast_arrays return value?

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

Warn or immidiately change readonly flag on broadcast_arrays return value?

mattip
In PR 12609 https://github.com/numpy/numpy/pull/12609 I added code to
emit a DepricationWarning when broadcast_arrays returns an array where
the output is repeated. While this is a minimal fix to the problem,
perhaps we should consider making the output readonly immediately instead?


- A deprecation cycle requires two changes to downstream user's code:
one to filter the deprecation warning, and another when we actually make
the change

- Writing to the repeated data will cause errors now.


What do you think, should we change the behaviour at all, and if so
should we depricate it over two releases or change it immediately?


The original issue is here https://github.com/numpy/numpy/issues/2705


Matti

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

Re: Warn or immidiately change readonly flag on broadcast_arrays return value?

Hameer Abbasi
Hi!

Broadcasting almost always returns a repeated output 
(except when all arrays are the same shape), that’s the entire point. I suspect this function is in fairly widespread use and will therefore cause a lot of downstream issues when repeating, so I’m -0.5 on a DeprecationWarning. A FutureWarning might be more appropriate, in which case I’m +0.2.

As for making the output read-only, that might break code, but most likely the code was erroneous anyway. But breaking backward-compatibility without a grace period is unheard of in this community. I’m +0.5 on it anyway. 🤷🏻‍♂️

Overall, a kind of hairy problem with no clear solution.

Best Regards,
Hameer Abbasi

On Tuesday, Dec 25, 2018 at 12:13 PM, Matti Picus <[hidden email]> wrote:
In PR 12609 https://github.com/numpy/numpy/pull/12609 I added code to
emit a DepricationWarning when broadcast_arrays returns an array where
the output is repeated. While this is a minimal fix to the problem,
perhaps we should consider making the output readonly immediately instead?


- A deprecation cycle requires two changes to downstream user's code:
one to filter the deprecation warning, and another when we actually make
the change

- Writing to the repeated data will cause errors now.


What do you think, should we change the behaviour at all, and if so
should we depricate it over two releases or change it immediately?


The original issue is here https://github.com/numpy/numpy/issues/2705


Matti

_______________________________________________
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
|

Re: Warn or immidiately change readonly flag on broadcast_arrays return value?

Juan Nunez-Iglesias
Probably this will cause a lot of groans, but I've definitely written code modifying `broadcast_to` outputs, intentionally. As such I am -1 on this whole endeavour. My preference on making arrays read-only is to have a very light touch if any. As an example, at some point `np.diag` started returning read-only views. Setting or modifying the diagonal of a matrix is a common operation, so this decision uglified my code (grab the diagonal, make it writeable, write to it, instead of `np.diag(M) = x`.

I'll admit that the times that I modified the `broadcast_to` output I felt rather hacky and sheepish, but the point is that it's unlikely that someone who is using this function doesn't know what they're doing, isn't it? I wouldn't have even thought to look for this function before I understood what broadcasting and 0-strides were. In fact, I use it *specifically* to save memory and use zero strides.

Matti, could you comment a bit on the motivation behind this change and why you feel it's necessary?

Thanks,

Juan.


On 25 Dec 2018, at 10:26 pm, Hameer Abbasi <[hidden email]> wrote:

Hi!

Broadcasting almost always returns a repeated output 
(except when all arrays are the same shape), that’s the entire point. I suspect this function is in fairly widespread use and will therefore cause a lot of downstream issues when repeating, so I’m -0.5 on a DeprecationWarning. A FutureWarning might be more appropriate, in which case I’m +0.2.

As for making the output read-only, that might break code, but most likely the code was erroneous anyway. But breaking backward-compatibility without a grace period is unheard of in this community. I’m +0.5 on it anyway. 🤷🏻‍♂️

Overall, a kind of hairy problem with no clear solution.

Best Regards,
Hameer Abbasi

On Tuesday, Dec 25, 2018 at 12:13 PM, Matti Picus <[hidden email]> wrote:
In PR 12609 https://github.com/numpy/numpy/pull/12609 I added code to
emit a DepricationWarning when broadcast_arrays returns an array where
the output is repeated. While this is a minimal fix to the problem,
perhaps we should consider making the output readonly immediately instead?


- A deprecation cycle requires two changes to downstream user's code:
one to filter the deprecation warning, and another when we actually make
the change

- Writing to the repeated data will cause errors now.


What do you think, should we change the behaviour at all, and if so
should we depricate it over two releases or change it immediately?


The original issue is here https://github.com/numpy/numpy/issues/2705


Matti

_______________________________________________
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
|

Re: Warn or immidiately change readonly flag on broadcast_arrays return value?

Marten van Kerkwijk
Hi Juan,

I also use `broadcast_to` a lot, to save memory, but definitely have been in a situation where in another piece of code the array is assumed to be normal and writable (typically, that other piece was also written by me; so much for awareness...). Fortunately, `broadcast_to` already returns read-only views (it is newer and was designed as such from the start), so I was alerted.

At the time `broadcast_to` was introduced [1], there was a discussion about `broadcast_arrays` as well, and it was decided to leave it writable - see the note at https://github.com/numpy/numpy/blob/master/numpy/lib/stride_tricks.py#L265. The same note suggests a deprecation cycle, but like Hameer I don't really see how that helps - there is no way to avoid the warning in the large majority of cases where readonly views are exactly what the user wanted in the first place. So, that argues for cold turkey...

The one alternative would be to actually warn only if one attempts to write to a broadcast array - apparently this has been done for `np.diag` as well [2], so code to do so supposedly exists.

All the best,

Marten


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