Skip to content

BUG?: np.array(a, copy=False) should check UPDATEIFCOPY flag. #9244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
charris opened this issue Jun 11, 2017 · 8 comments
Open

BUG?: np.array(a, copy=False) should check UPDATEIFCOPY flag. #9244

charris opened this issue Jun 11, 2017 · 8 comments

Comments

@charris
Copy link
Member

charris commented Jun 11, 2017

Currently

In [1]: a = np.ones(10)[::2]

In [2]: a.flags
Out[2]: 
  C_CONTIGUOUS : False
  F_CONTIGUOUS : False
  OWNDATA : False
  WRITEABLE : True
  ALIGNED : True
  UPDATEIFCOPY : False

In [3]: b = np.array(a.flat, copy=False)

In [4]: a.flags
Out[4]: 
  C_CONTIGUOUS : False
  F_CONTIGUOUS : False
  OWNDATA : False
  WRITEABLE : False
  ALIGNED : True
  UPDATEIFCOPY : False

The reason is that a.flat.__array__ is called which returns a new array with UPDATEIFCOPY and locks a by clearing the WRITEABLE flag. Because b hangs onto a reference of the uncopied array, it does not get deallocated, hence does not re-enable the WRITEABLE flag.

Whether this is a bug or a feature is debatable, but given the usual use of asarray (copy=False), I think that a copy of the array should be made when UPDATEIFCOPY is set.

@charris charris changed the title BUG: np.array(a, copy=False) should check UPDATEIFCOPY flag. BUG?: np.array(a, copy=False) should check UPDATEIFCOPY flag. Jun 11, 2017
@seberg
Copy link
Member

seberg commented Jun 11, 2017

Why does it set the UPDATEIFCOPY flag though? It needs it internally for in-place operations probably, but I doubt it should do it on __array__(). Can't see much of a reason anyway, while you might be able to do fun hacks with it, in the end they only safe a single arr.flat[...] = ... assignment.

About creating a copy, maybe it is a good idea, could be interesting to add a warning at least for a test to see if that actually happens.

@eric-wieser
Copy link
Member

eric-wieser commented Jun 12, 2017

What is the purpose of UPDATEIFCOPY? Under what circumstance is it useful?

@charris
Copy link
Member Author

charris commented Jun 12, 2017

Personally, I think it should be hidden away in the numpy internals where I can be a useful tool. Unfortunately, it is exposed by the np.flatiter method. It is used there is for creating 1-D contiguous copies of non-contiguous arrays, which can then be indexed, etc, with the result copied back to the original array.

@pv
Copy link
Member

pv commented Jun 12, 2017 via email

@njsmith
Copy link
Member

njsmith commented Jun 12, 2017

In the nditer case it makes some sense though, because it gives you a chunk of an array, lets you modify it, and then when you advance the iterator to the next chunk it does (should) do the copy back for the previous chunk. Probably we should make it so that the copy back is triggered unconditionally by the iterator advancing rather than relying on the GC to trigger it, but fundamentally it makes sense.

For this .flat thing.... I get that there are backcompat considerations, but it really doesn't make sense to me. Sure the user might have passed copy=False, but the vast majority of the time we interpret that to mean "return a view if you can, otherwise make an independent copy". asarray(np.arange(3), dtype=float) doesn't give you an UPDATEIFCOPY array.

charris added a commit to charris/numpy that referenced this issue Jun 13, 2017
Before this, one would have

    In [1]: a = np.ones(10)[::2]

    In [2]: a.writeable
    Out[2]: True

    In [3]: b = np.array(a.flat, copy=False)

    In [4]: a.writeable
    Out[4]: False

because b held a reference to the updateifcopy array a.flat.__array__(),
keeping it from being deallocated and resetting a to writeable. That was
because the array constructor returned a.flat.__array__() directly
instead of making a copy. It seems that in normal usage it would be
better to return a copy in these cases and this commit makes that
change.

Closes numpy#9244.
@charris
Copy link
Member Author

charris commented Jun 13, 2017

See #9249.

@seberg
Copy link
Member

seberg commented Jun 13, 2017

Frankly, I am also not sure that the writeable flag should be unset (basically as a thread safety lock?) as well? And I doubt we do this in other places, that also may end up releasing the GIL during e.g. a larger copy back process....

EDIT: It is not really a lock of course, which is odd, but I suppose you could argue it is useful as a "don't do this" warning (plausibly, it seems unlikely to actually happen much/at all in practice)

@mattip
Copy link
Member

mattip commented Sep 19, 2017

closable after PR #9447?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants