Julia: reshape and push! inconsistent behavior

[cossio]

Consider the following example:

xmat = randn(5,5)
xvec = reshape(xmat, 25)
push!(xvec, 3)
xvec[1] = 0.0
xmat[1] # not zero

Here push! has decoupled xmat and xvec, though one would expect that a reshaped array always point to the same memory as the original array. I think the push! here should raise an error.

Contrast the example above with the following (source: https://discourse.julialang.org/t/puzzling-reshape-and-push/28326/2):

xvec = rand(4)
xmat = reshape(xvec, 2, 2)
push!(xvec, 1.0)  # ERROR: cannot resize array with shared data

To be consistent it seems to me that my first example above should also be an error.

[yuyichao]

This is an intentional hehavior. Changing this would be a breaking change.

Even though the implementation is different, just like subarray and reinterpreted arrays, you can't expect the original array and the new array to be completely symmetric so "consistency" isn't really an argument here. The real question then is which one is a better behavior.

In fact, both can be implemented from the other (the push does a copy implicitly) so the two behavior are basically equivalent (edit: apart from that the user code currently don't have enough information to do this copy automatically). Having a more explicit error could be a good thing since aliasing issue are a bit hard to debug so a change in 2.0 would probably be fine.

[KristofferC]

This is an intentional behavior.

According to what? This really feels like implementation detail leaking out and should be fair game to change. E.g. push! doesn't document that is unshares data when the buffer is owned by another array.

[yuyichao]

According to what?

According to this being explicitly implemented in the code. It's not a property of a naive implementation that didn't take this into account and it's definitely not a leak of implementation detail.

should be fair game to change.

No. This behavior is something someone can depend on. Failure to mention in the doc is just an argument to fix the doc. Raising an error in a case that previous doesn't is not going to be a minor change for sure.

[yuyichao]

Also, similar to many other aliasing issues, there are difference between users that care and users that doesn't. A user that just want to create an interpreted array before using it as a normal one is not going to care if the alias breaks or not and should be allowed to resize the new array. If the error is raised on the new array for resizing, there should be a way to support resizing a possibly shared array without making unnecessary copy when the array isn't shared.

[StefanKarpinski]

We can and do make technically breaking changes in minor releases under the term “minor change” so long as they are 1) undocumented, 2) unlikely to be relied upon on real code, 3) not actually relied upon in any packages according to PkgEval. This seems likely to qualify on all three counts. Especially since any code that relies on this is either broken already or will break with a fairly clear error message.

[yuyichao]

Especially since any code that relies on this is either broken already

Why? Not everyone cares about the old array after the new array is created.

or will break with a fairly clear error message.

But doesn't necessarily have an easy fix. See my comment above.

[yuyichao]

And FWIW, this is tested.

[StefanKarpinski]

If you want to join the triage call and discuss it, it’s on Thursday at 2:15 pm Eastern. It’s hard to come up with a legitimate use case for this “feature”.

[yuyichao]

Discussion on github is way better logged.

It's hard to come up with any use case for many features on the spot. FWIW, it's hard for me to come up with a case that I want to resize to a vector either (edit: actually just remembered one from what I'm doing now, see below). However, this is all about combining usage of independent code. As long as you have a usecase that require you to create a vector, there should be cases you want to feed that vector to code that will resize that vector.

Things that are closely related to what I'm doing atm will be loading experiemental data from an external file. The loader of the file may return a matrix (since that's how the matlab code that produces it saves it in the MAT file) and it's possible that I want to use it as a vector. (and I do have reshaping code to do this sort of things). I also do mutate/combine/append these data before processing them together so even though I'm not 100% sure if I'm relying on this now, I can easily see a case where I am. In this case I obvously also want to decouple the loading code from the processing code since there are many different ways to load and process data.

Note that the property in this case that makes this a legitimate usecase is that I don't care about the old data. I need the new data to be in the format that I can process, the old data is essentially garbage once the new one is there.

[yuyichao]

Oh, and another case that I've done before. When creating a vector that is a flattened out matrix, I would create the matrix and then reshape so that I can push to the vector later. I'm 99% sure this comes from a case where I'm doing something related to density matrix (and treating the density matrix as vector to do operation on it) while also need to append a few extra dimentions to take care of other part of the system.

Again, I'm not saying such thing couldn't be don't with the new behavior. However,

1. It's wrong to say that such usecase must not exist since one can't come up with one.
2. It's particularly bad in this case since the fix could be no where near the error. The error will be raised when one try to push to the vector but the correct fix would be copying the initial array rather than doing anything with the push!.

The current behavior is also essentially a COW (of array metadata) so erroring in all cases will cause unecessary copies unless a new function is added to do the COW explicitly. Even then, in order to support any input, generic code will be required to always call that function before resizing an array which is even worse than doing that automatically as what we do now.

[JeffBezanson]

I think we should make both cases work like the first example --- remove the error and unshare the data on push! in all cases. Triage seems ok with that.

Of course, SubArray is not able to emulate that though.

[StefanKarpinski]

Triage discussion with @vtjnash favors what @JeffBezanson said.

[StefanKarpinski]

Someone ought to fix this issue if they're interested. Triage has already decided.

[expnn]

Sometimes it is weired to raise an exception when pushing an array. It prevents the users from pushing new elements into an array after calling a function that performs some shared data operations, like reshape.
The following snippet is an example. It show that the array v is marked shared after calling a function, even though the sharing data array (i.e., the local x in func) can be safely garbage collected after the calling of func.
It seems there is no way to unmark an array that it is not shared any more. After explicitly calling the gc(), the push! is still forbidden.

function func(a)
    x = reshape(a, 2, 2)
    z = x * x
    return z
 end

v = rand(4)
y = func(v)
push!(v, 1.0)  # ERROR: cannot resize array with shared data

GC.gc()
push!(v, 1.0)  # ERROR: cannot resize array with shared data