Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Clean up diagonal #280

Merged
merged 15 commits into from

4 participants

@njsmith
Owner

This pull request implements a transition scheme so that someday PyArray_Diagonal (and its interfaces numpy.diagonal, ndarray.diagonal, etc.) can return a view rather than a copy.

The proposed scheme is:

  • Numpy 1.7: PyArray_Diagonal returns a copy (as before), but writes to this copy produce a DeprecationWarning.
  • Numpy 1.8: PyArray_Diagonal returns a view, but writes to this view produce an error.
  • Numpy 1.9: PyArray_Diagonal returns an ordinary view, transition complete.

The 1.7 DeprecationWarning is implemented by this pull request, which adds a new hidden flag called NPY_ARRAY_WARN_ON_WRITE ('hidden' in the sense that it isn't exposed to Python by the flagsobject, though you can still see it from Python if you want by checking arr.flags.num & 0x10000). This flag is propagated through views, and, if set, causes a warning to be issued on the first write to an array.

I recommend looking at the individual commit diffs rather than the overall pull-request diff, since each commit is a self-contained unit. In particular the first 3 are generally useful cleanups independently of whatever we do with PyArray_Diagonal, the 4th implements the actual change, and the 5th documents it.

njsmith added some commits
@njsmith njsmith Consolidate all array writeability checking in new PyArray_RequireWri…
…teable

This is mostly a code cleanup, but it does have a user-visible effect
in that attempting to write to a unwriteable array now consistently
raises ValueError. (It used to randomly raise either ValueError or
RuntimeError.)

Passes numpy.test("full").
cbce4e6
@njsmith njsmith Funnel all assignments to PyArrayObject->base through a single point
This patch removes all direct assignments of non-NULL values to the
'base' field on PyArrayObjects. A new utility function was created to
set up UPDATEIFCOPY arrays, and all assignments to 'base' were
adjusted to use either PyArray_SetBaseObject or the new
PyArray_SetUpdateIfCopyBase.

One advantage of this is that it produces more consistent error
handling. This error handling revealed a bug in the nditer code,
which this patch does *not* yet fix.

The bug is that npyiter_new_temp_array sometimes (when dealing with
reversed axes) creates an array and then returns a view onto it. But,
npyiter_allocate_arrays assumes that the array returned by
npyiter_new_temp_array can have UPDATEIFCOPY set, which requires
reassigning its 'base' field. Previously, this meant that the
temporary array leaked. Now, it produces a ValueError.

This code path doesn't seem to actually be hit very often; only one of
the nditer tests fails because of the change. See
   numpy/core/tests/test_nditer.py:test_iter_array_cast_buggy
6a90ada
@njsmith njsmith Add a test for ndarray.diagonal()
All 'expected' values were computed using numpy 1.5.1, so we can be
pretty sure that the recent rewrite didn't change any results.
d403fed
@njsmith njsmith Transition scheme for allowing PyArray_Diagonal to return a view
PyArray_Diagonal is changed to return a copy of the diagonal (as in
numpy 1.6 and earlier), but with a new (hidden) WARN_ON_WRITE flag
set. Writes to this array (or views thereof) will continue to work as
normal, but the first write will trigger a DeprecationWarning.

We also issue this warning if someone extracts a non-numpy writeable
view of the array (e.g., by accessing the Python-level .data
attribute). There are likely still places where the data buffer is
exposed that I've missed -- review welcome!

New known-fail test: eye() for maskna arrays was only implemented by
exploiting ndarray.diagonal's view-ness, so it is now unimplemented
again, and the corresponding test is marked known-fail.
bea52bf
@njsmith njsmith Document the PyArray_Diagonal transition scheme. 0812564
doc/release/2.0.0-notes.rst
@@ -148,36 +183,20 @@ New argument to searchsorted
The function searchsorted now accepts a 'sorter' argument that is a
permuation array that sorts the array to search.
+C API
+-----
+
+New function ``PyArray_RequireWriteable`` provides a consistent
+interface for checking array writeability -- any C code which works
+with arrays whose WRITEABLE flag is not know to be True a priori,
@teoliphant Owner

"not known to be True"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njsmith njsmith Typo fix
Thanks to Travis for catching it.
c247305
@mwiebe

This is a good idea. I suspect PyArray_EnsureWriteable might be a slightly more intuitive name for this.

numpy/core/src/multiarray/arrayobject.c
((8 lines not shown))
+ * Returns 0 on success, -1 on failure.
+ */
+NPY_NO_EXPORT int
+PyArray_SetUpdateIfCopyBase(PyArrayObject *arr, PyArrayObject *base)
+{
+ if (base == NULL) {
+ PyErr_SetString(PyExc_ValueError,
+ "Cannot UPDATEIFCOPY to NULL array");
+ return -1;
+ }
+ if (PyArray_BASE(arr) != NULL) {
+ PyErr_SetString(PyExc_ValueError,
+ "Cannot set array with existing base to UPDATEIFCOPY");
+ goto fail;
+ }
+ const char *msg = "cannot UPDATEIFCOPY to a non-writeable array";
@mwiebe Owner
mwiebe added a note

Variables must only be declared at the beginning of blocks in the version of C Numpy supports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mwiebe

I created a bug report http://projects.scipy.org/numpy/ticket/2135, and fixed the bug. Can you rebase from master and reenable these tests?

@mwiebe
Owner

After renaming PyArray_RequireWriteable to PyArray_EnsureWriteable, fixing the C compile error, and reenabling the nditer test, I'm ok with merging this.

@njsmith
Owner

I'm -0 on the "EnsureWriteable" name -- to me "ensure" means that it will take efforts to make it true. E.g. asarray "ensures" that the input is an ndarray. One can imagine a function that passes through writeable arrays and makes temporary copies of unwriteable ones, but that isn't this function.

So I like RequireWriteable, or perhaps MustBeWriteable.

@njsmith
Owner

Okay, I think this is ready to merge (modulo bikeshedding about the function name).

@njsmith
Owner

Spoke too soon, but now it should be ready :-)

@teoliphant
Owner

If one more opinion helps, PyArray_RequireWriteable seems fine to me. I agree that EnsureWriteable seems to indicate that "something will be done to try and make it writeable" while in this case it's a check. I would be +0 on PyArray_CheckWriteable.

@njsmith
Owner

The Python C API convention is that "Check" is used for functions that return a boolean, though...

@charris

All this function does is raise an error message, I don't think something like this belongs in the API. Make it an inline helper function somewhere if you really think you need it.

@charris

This doesn't improve the original code, I think it makes it less readable. Ditto for the rest of the use cases.

@charris

Multiline comments like so:

/*
 * blah
 */

[Fixed. -- Nathaniel]

@charris

This isn't portable, all declarations need to be up top.

[Yeah, Mark caught this too -- it was already fixed in a later commit. -- Nathaniel]

@charris

I think there is a better case for putting this in the API than for the previous function, but I'm not convinced. The documentation could use a bit more expansion.

[Docs have been expanded: 6d36127#L1R71 -- Nathaniel]

@charris

Needs blank line up top. Ditto for rest.

[Thanks, fixed. -- Nathaniel]

@charris

This is rather verbose, and need '\n' to break it up in any case. I think it best to keep these messages to 1, maybe 2 lines.

[Well, I made it shorter: 6d36127#L1R770

If you can see how to make it even shorter while still being useful, please let me know :-). I do tend to think we should error on the side of intelligibility over terseness for things like this -- it's not like the bad old days where error message space is limited! -- but of course the words should carry their weight. -- Nathaniel]

@charris

PyErr_WarnEx isn't available for python 2.4, use PyErr_Warn in that case. See the DEPRECATE macro in numpy/core/include/numpy/ndarrayobject.h.

[Thanks, fixed. -- Nathaniel]

@charris

Is it guaranteed that this loop will hit the break condition?

[Yes, it walks through the ->base chain of the passed in object, and stops when it hits an object whose base is NULL or a non-ndarray object. So the only way it could get stuck would be if we had a ->base pointer loop, which would require a bug somewhere else. In fact PyArray_SetBasEObject explicitly refuses to create such loops. -- Nathaniel]

@charris

Interesting idea. I'm a bit leery of using up a flag for this, is there another way that you could do this?

[Well, I need to stash a bit somewhere in PyArrayObject, so this seems like the obvious place... but, I'm not too worried for two reasons. First, we have 32 flag bits, and this is only the 15th. (And some of the maskna-related ones might even go away.) And second, this bit can be recycled after the 1.7 release, if we want -- it's purely for internal use. To make extra sure of this, though, I just added an #ifdef _MULTIARRAYMODULE guard on it so that the symbol isn't even available to user code. -- Nathaniel]

njsmith added some commits
@njsmith njsmith Clean up PyArray_Diagonal changes based on Chuck's feedback
- NPY_ARRAY_WARN_ON_WRITE flag definition is protected by #ifdef
  _MULTIARRAYMODULE, to make totally sure that we can recycle the flag
  bit later.
- Improve docs for PyArray_SetUpdateIfCopyBase.
- Make the deprecation warning for writes to the diagonal array
  somewhat more terse.
- Use DEPRECATE macro instead of calling PyErr_Warn/PyErr_WarnEx
  directly.
- Comment formatting fixed.
6d36127
@njsmith njsmith Don't assume ctypes is available, it's not true on Python 2.4 58392b0
@njsmith njsmith Fix diagonal-related warnings and tests on Python 3 ac4ce7b
@njsmith
Owner

Chuck: I wrote short responses to many of your comments as "edits" to the comments, so as to keep the discussion overview page readable (since it doesn't seem to do threading). But I guess you probably don't get notifications that way, so FYI.

Also, there were a few more Python version compatibility issues that I hadn't noticed, but this PR now passes tests on python 2.4, 2.5, 2.6, 2.7, 3.1, 3.2.

Bottom line: I think the main issue left is the question of whether PyArray_RequireWriteable and PyArray_SetUpdateIfCopyBase should be exposed in the API.

For RequireWriteable: Mark likes it; Chuck doesn't. I'm inclined to leave it public myself. Sure, it's simple enough that extension authors can open-code it everywhere they want it, but it fulfills the two main criteria I can think of for an abstraction: (1) it captures a conceptually coherent idea ("always call this before writing to an array"), (2) while the operation it performs is simple, it's not too simple to get wrong, as evidence by the number of random different exceptions that numpy used to throw depending on which code path discovered the non-writeability.

For UpdateIfCopyBase: I just made this public because it parallels PyArray_SetBaseObject. I'm not sure it's a great idea for user code to ever mess with UPDATEIFCOPY, but eh. UPDATEIFCOPY isn't going anywhere.

So I guess I'm +0 on both staying public, but I really don't care that much. If pulling one or both of them out of the API will let us move on then let's do it :-)

@charris
Owner

Hi Nathaniel,

The downside of the edits is that they don't get mailed out. I'd like another opinion on the API/flag issues before committing this. I really don't like the first function being in the API and would prefer the code as it is because it is more self documenting. I don't much like passing the error message to it either; it really looks to me like something that belongs in one of the include files.

@mwiebe Mark, could you give your opinion on the functions in the API and using a flag slot for a deprecation warning?

@mwiebe
Owner

I like the RequireWriteable function because it provides a central place that anything affecting writeability can go. I still don't like the name, "if (RequireWriteable(a) < 0) ..." reads to me like it's asking whether 'a' requires something writeable up until the "< 0" part. Maybe one of "if (RaiseUnlessWriteable(a) < 0) ..." or "if (RaiseIfNotWriteable(a) < 0) ..." is more clear and not too clunky?

The change to UpdateIfCopyBase is good - the additional validity checks caught a clear bug in nditer. It's probably better if this one becomes private, though, as the way to get this set from third-party code is PyArray_FromAny with the NPY_ARRAY_UPDATEIFCOPY flag.

@mwiebe
Owner

A flag seems to me like the most reasonable way to trigger this warning. Maybe the flag's #define could go in 'src/multiarray/arraytypes.h' instead of the public header 'ndarraytypes.h' with the include guard? It would be nice to avoid putting things people aren't supposed to use in the headers people will read to see what the API provides.

@charris
Owner

How about AssertWriteable? Assert also comes with an expectation of an error message.

@charris
Owner

Maybe the flags should be divided into public/private, with the private ones starting at the high end. It's a shame the flags slot is an integer instead of something of fixed size, but I think it will be at least 32 bits on all the architectures we support.

@teoliphant
Owner

+1 on public/private flag distinction and +1 on AssertWriteable (but I don't have a strong opinion on the API name).

@mwiebe
Owner

+1 on AssertWriteable too

njsmith added some commits
@njsmith njsmith Move internal NPY_ARRAY_WARN_ON_WRITE flag into an internal header. baaf181
@njsmith njsmith Rename PyArray_RequireWriteable to PyArray_FailUnlessWriteable
Also clean up its API slightly so that the caller passes in a name
describing the array being checked which is used to generate an error
message, rather than writing an error message from scratch.
51616c9
@njsmith
Owner

Okay, so I've:

  • Moved the flag bit to (1 << 31), and moved the definition to multiarray/arrayobject.h (baaf181)
  • Renamed RequireWriteable to FailUnlessWriteable (because it doesn't raise an AssertionError!), and changed it so that instead of passing the error message itself, you pass a noun phrase describing the array being checked, and we then use this to generate an error message in a standard format. (51616c9)

I also started to remove SetUpdateIfCopyBase from the API, but that made it inaccessible to NA_OutputArray in numarray/_capi.c. I don't really understand what this code is doing, though. Would PyArray_FromAny work? Guess this is a question for @teoliphant.

@charris
Owner

Actually, C assert doesn't raise an assertion error, it calls abort. If I may appeal to the idea of consensus, about which there was philosophical discussion, there was pretty much a consensus on AssertWriteable.

@teoliphant
Owner

@njsmith Is the question about what NA_OutputArray does? It's an old numarray API that motivated the updateifcopy notion in NumPy in the first place. NA_OutputArray is basically a function that makes sure you get an array that satisfies particular requirements (e.g. contiguous and aligned) of a particular type but that any changes to this array will get propagated back to the other array when the new array returned is deallocated. The "base" array to be copied to is set read-only until the copy-back is done.

This function does not need SetUpdateIfCopyBase as it uses SetBaseObject instead.

@teoliphant
Owner

@njsmith With consensus being such a rare even these days among major committers to NumPy :-), perhaps it would be helpful to name the C-API PyArray_AssertWriteable even if there is no Python Assertion Error raised inside.

@njsmith
Owner

@teoliphant: Re: NA_OutputArray: NA_OutputArray uses SetBaseObject in master... but the whole reason I had to introduce SetUpdateIfCopyBase is that all the numpy code dealing with UPDATEIFCOPY was avoiding SetBaseObject because SetBaseObject+UPDATEIFCOPY doesn't actually work. SetBaseObject collapses the base chain, so in current master we have:

  a = np.zeros((10, 10))
  a_slice = a[0, :]
  b = NA_OutputArray(a_slice)
  # Now:
  b.shape == (10,)
  b.flags.updateifcopy == True
  b.base is a # not a_slice! This will not work!

I took another look, and the easy fix is to just make NA_OutputArray call NA_IoArray, which makes strictly stronger guarantees and goes via PyArray_CheckFromAny. Technically this introduces a tiny inefficiency, but it's simple, obviously correct, and lets us give numpy proper a better interface, which all seem like higher priorities given how little usage the numarray compat code has these days. I'll push the change so you can check it.

@njsmith
Owner

@charris: Sorry, yes, that was too compressed. I'm not trying to
overturn consensus here; I just judged that probably everyone would be
happy with FailUnlessWriteable, so I might as well suggest it in code
form. Obviously that was over-optimistic.

If you all want AssertWriteable, then whatever, say the word and I'll
switch the code over.

But, I do think it's an actively misleading name. "Assert" is a
term of art that always means "I don't think this invariant can ever
be false, if I'm wrong then I screwed up somewhere". So you don't use
asserts for input checking, because detecting and reporting invalid
input is a standard requirement of an interface; invalid input doesn't
indicate a bug in your code. C and Python asserts are different in
detail, but this is why they both have minimal error reporting
(abort() vs. throwing an exception that no-one ever catches and
usually has no useful error message attached), why both can be
compiled out depending on optimization level, etc.

Plus, in Python we re-use asserts for testing, and that gives a very
strong convention that functions named "assert " will check
and then raise an AssertionError if it fails
(cf. assert_equal, assert_raises, there are dozens of the things).

FailUnlessWriteable seems to satisfy Mark's concern (being
more-or-less his suggestion :-)) and be an API that we won't require
a "don't be confused!" warning in the docs, which seems like a good
combination to me.

@charris
Owner

Nathaniel, I don't think people have strong feelings about the name, but the way to do it is to say "I'd prefer xxx rather than yyy because ..." and ask for response. There are lots of other possibilities, CheckWriteable for instance.

@teoliphant
Owner

The name PyArray_FailUnlessWriteable is fine with me, by the way.

@teoliphant
Owner

@njsmith Going with NA_IoArray is probably fine, I just don't think it's necessary. In general you can't use PyArray_SetBaseObject with Updateifcopy (because of the chain compression), but it's use in NA_OutputArray is fine because you are starting with an obviously just-created "empty" array, so in that case PyArray_SetBaseObject is equivalent to PyArray_SetUpdateIfCopyBase.

@njsmith
Owner

@teoliphant: Cool, I'll leave that patch in then and go ahead and un-expose SetUpdateIfCopyBase. (FYI, the bug is if the input array to NA_OutputArray already has a non-null .base attribute, so I don't think it matters that the array it's setting the .base attribute for is newly created, in principle you can hit the bug. Not that it's clear whether there still exists even a single user of NA_OutputArray, there aren't any in tree...)

@njsmith
Owner

@charris: Yes, like I said, I was hoping we would skip straight to consensus on FailUnlessWriteable and save some time, because having long discussions about everything isn't very practical. Apparently I misjudged your position, though, so I'm happy to have the discussion.

I'd prefer FailUnlessWriteable rather than AssertWriteable because of what I said. What do you think?

(FYI, CheckWriteable was already discussed upthread.)

@charris
Owner

I think AssertWriteable reads better. The long discussion could have been avoided if you had accepted the three plus votes, so don't put that on me. Consensus is unworkable without a bit of compromise.

@teoliphant
Owner

@njsmith Doh! You are right. I gave a confusing explanation. It is the fact that a might have a base it's already pointing to that is the problem. ret should be pointed to 'a' as it's base without any compression of the base-object.

If we don't want to hard-code access to the ->base pointer, then we need the SetIfUpdateIfCopyBase (but, it should be called something more general because it's just basically arg1->base = arg2 with no base traversal....). UpdateIfCopy is one use case for not wanting base-traversal, but there may be others, so just call it SetImmediateBase or something.

@njsmith
Owner

@charris: I'm not putting anything on you; I'm just noting that we were both wrong about whether we had consensus. I'm also happy to compromise, like I said already, but I'm not going to just pretend an engineering problem doesn't exist in favor of making discussion easier; that would be irresponsible.

If you think that AssertWriteable reads sufficiently better to override API clarity then that's a pretty strong statement in favor of euphony, so maybe we should think harder about how to get both. InsistWriteable? DisallowUnwriteable? DemandWriteable?

@njsmith
Owner

@teoliphant: How about we just let NA_OutputArray call NA_IoArray (and thence PyArray_FromAny), and make SetUpdateIfCopyBase private like @mwiebe suggested (#280 (comment)). That way we can always change it later if it turns out to matter. Works?

@teoliphant
Owner

@njsmith I don't think that is the appropriate thing to do. There really does need to be an API for setting the base object that does not compress the bases. This should have been added when PyArray_SetBaseObject was added in the first place, but for some reason was not. Given that we need that API anyway, we should just use it in NA_OutputArray.

@njsmith
Owner

@teoliphant: I'm afraid I'm lost now. Why would we ever need such an API? Does it really make sense to be adding an API with no users just in case some come along later?

@teoliphant
Owner

@njsmith If we really don't want to extend the API, then just put the code in NA_OutputArray to set the base object directly: i.e. PyArray_BASE(ret)=(PyObject *)a and deprecate NA_OutputArray pointing people to PyArray_FromAny instead. But, I still think we need an API that is exactly equivalent to PyArray_BASE(ref)=(PyObject *)a. I'm still not sold that there is a problem with that spelling in the first place.

@teoliphant
Owner

@njsmith The PyArray_SetBaseObject API is the new API --- very recently added, I guess by Mark, but I'm not sure. It did not receive a lot of discussion. This API compresses the bases (i.e. finds the object which actually owns the data). In the past, the base always pointed to the most-recent object from which the array derived. Doing this compression has certain advantages, but it might not always be the right thing to do (like in this case). I'm not so sure that all the places where PyArrary_SetBaseObject(arg1, arg2) is now used, it should not still be PyArray_BASE(arg1) = (PyObject *)arg2. So, my point is that if you are going to add the 1 API, you have to either accept the spelling for the other use case as PyArray_BASE(arg1) = (PyObject *)arg2 or add another API call.

I would advocate just accepting the spelling PyArray_BASE(arg1) = (PyObject *)arg2. I still don't really understand what we are buying by "function calls" instead. I am not convinced that it is actually beneficial in the long run.

In summary: just change the PyArray_SetBaseObject(ret, a) to PyArray_BASE(ret) = (PyObject *)a;

@njsmith
Owner

@teoliphant: I see, right. It looks like Mark originally added it in b7cc20a (July 2011) as part of the direct field access deprecation stuff he did.

I'm also skeptical about the value of using function calls everywhere as a matter of course, but in this particular case I think it gives concrete advantages. The SetBaseObject API has better error checking, it gives us the base compression trick (which I guess saves memory usage in some cases), and -- the reason why we're talking about this at all as part of this PyArray_Diagonal change -- it's the only reliable way to detect views, so it gives a nice unobtrusive place to propagate the WARN_ON_WRITE flag to views. Can't do that with direct field access, whether or not it's wrapped in a macro like PyArray_BASE.

IIUC, there are exactly three cases where the ->base field is used:

  • It can pin a PyArrayObject in memory when making a view
  • It can pin an arbitrary non-ndarray PyObject in memory
  • It can designate the destination of an UPDATEIFCOPY writeback

We're currently handling all three of these cases correctly without using the new API you're suggesting. A quick skim through the existing calls to SetBaseObject seem to show that they all fall into the first two categories, which means that SetBaseObject is fine for them. I would definitely feel better about adding an API for this is you could come up with even one example of what it would be good for, besides UPDATEIFCOPY.

Well, anyway, there are a few possible ways forward:

  • Just use direct field assignment in NA_OutputArray. This means that it'll be buggy with respect to WARN_ON_WRITE and possibly other future changes to base object handling, but maybe we just don't care if NA_OutputArray is a bit buggy.
  • Remove base compression from SetBaseObject. This means that in a view chain, the intermediate PyArrayObject structs will stay pinned in memory so long as the outermost view object lives, which is wasteful of memory but probably unmeasurable in practice.
  • Add a new API function that acts like SetBaseObject but without base compression. This would be a pretty trivial and unused API function to support, though.
  • Use fa157f2255fee20cccb9a66a250d0c768f7d70fb to kick this down the road until someone comes up with another case where avoiding base compression is actually needed. In the mean time the PyArray_BASE(arr) = ... spelling will continue to exist...

Thoughts?

@teoliphant
Owner

@njsmith I am now confused. The API we are talking about is not mine, it is the one in this PR: SetUpdateIfCopyBase. The UPDATEIFCOPY facility is an important one. PyArray_SetBaseObject currently does not support that use-case which is why everywhere in current master that the use-case is needed, direct setting of the base member is used: including NA_OutputArray. That's why you introduced the API that we are talking about in order to intercept this use-case of setting the base object. You wanted to create the function-call internal to NumPy inorder to propagate your new WARN_ON_WRITE flag.

I presume you are going to keep this internal function call. If that is the case, then we must export it as well, because NA_OutputArray is just a simple use-case that mimics what other consumers of the NumPy API might be doing on their own: Create an empty array to hold data in a contiguous fashion (that you might be using for an output array when you call some C or Fortran code) and then attach another malformed NumPy Array to the "base" object in order to propagate any changes back to the malformed output array when the routine finishes.

PyArray_FromAny does not do this. It takes an array and returns one that satisfies certain requirements (allowing the UPDATEIFCOPY flag to be set if changes to the result should be copied back to the original). I suppose it could be used in a round-about way so that one could call PyArray_Empty and then PyArray_FromAny, but that is a very indirect way to do a very simple thing and much too confusing.

This is an important and documented use-case. If you want to intercept the simple operation of setting the base object for UPDATEIFCOPY functionality, then you will need to also introduce an API for consumers of NumPy to use if they want to help their code not be buggy.

This seems very cut and dry to me and I feel pretty strongly that if you are going to replace the simple setting of the base object in PyArray_FromAny and other places in the NumPy code base with a function call, then this function call must also be exported to other downstream consumers so they can do the "right" thing as well.

Now, another possibility is to simply add a flag to the PyArray_SetBaseObject API which toggles whether or not base chain compression happens. This flag could be called updateifcopy. But, I think Mark would prefer that there be another API because I know he doesn't like re-using the base object for two purposes (but I don't like having two structure members when one will do because the use-cases are orthogonal).

On a related note, I can't think of another case where setting the base-object does not really benefit from compression, but that doesn't mean they don't exist. There has not been base chain compression until 1.7 and so I'm not really sure if consumers have been relying on the fact that say a slice of the array has a base-object that points to the original array. They certainly could have been. I've done that from time to time in simple tests and examples (i.e. looked at the shape of the base-object which resulted from a slice operation). This will no longer be the case if the original array used memory from a buffer. There are work-arounds for all these use cases, but that doesn't mean we aren't going to create issues in people's code moving to 1.7.

So, this new behavior of PyArray_SetBaseObject has to be very well advertised to avoid downstream problems. If the flag exists to toggle compression of the bases, then it would be an easy thing to change this behavior. But, this also suggests that another API is useful to differentiate the updateifcopy use case from the chain compression use-case. Updateifcopy won't ever want to compress the chain, but it also has other desired features (like ensuring propagation of your WARN flag).

So, I am supportive of:

  • exporting the new API call you introduced
  • adding a flag or enumerated argument to PyArray_SetObjectBase that allows it to be called with different behaviors: * COMPRESS * NO_COMPRESS * UPDATEIFCOPY

My preference is the last one because it also allows for consumers to not compress the base argument if they do not want to. Of course, those consumers could continue to use PyArray_BASE(arr) directly which is why the first case is acceptable, but it seems cleaner to put all BaseSetting into a single function call.

@mwiebe
Owner

For reference here's the bug report that the chain compression in PyArray_SetBaseObject fixed:

http://projects.scipy.org/numpy/ticket/466

@njsmith
Owner

@teoliphant: I see! I thought you were saying that we needed all three of (1) SetBaseObject, (2) SetUpdateIfCopyBase, (3) JustSetTheBaseWithoutCompression, as separate APIs.

I was all set to say screw it, let's just disable base compression across the board, but I guess that bug Mark points to is legitimate. Not sure anyone really needs to reverse an array a million times, but okay, presumably they ran into it somehow...

So. I actually agree with Mark (or at least your characterization of his opinion :-)), and think the overloading of "base" is a bit ugly (and it's the basic cause of the bug nditer bug this change discovered -- it was trying to have an array be a view and UPDATEIFCOPY simultaneously). I'm also generally opposed to adding interfaces "just in case" someone ever finds a use for them, even though we can't actually think of one. So on balance I prefer option (1), of making SetUpdateIfCopyBase into a real API. Sound good? (esp. a question for @mwiebe, since you raised the original objection to this being public?)

As for the question of what to do with base compression in general, I don't know, and it isn't really relevant to PyArray_Diagonal -- can we split that off into a separate bug or PR?

@teoliphant
Owner

@njsmith Great! I know I have been confusing in my part of the discussion by commenting on related API issues. I definitely did not want three API calls. I'm completely fine with just the two (and the fact that people can still call PyArray_BASE if they really, really want to to avoid compressing bases).

I won't argue that overloading "base" for two purposes is not ugly --- it just was better than introducing a new field just to support Numarray's UPDATEIFCOPY use-case.

I'm +1 on the public API for PyArray_SetUpdateIfCopyBase.

We can move the discussion of what will happen when PyArray_SetBaseObject compresses the base-chain on list.

@njsmith
Owner

Okay, so unless someone else objects, I'll plan to revert the NA_OutputArray change and leave SetUpdateIfCopyBase exposed in the API.

Anyone want to weigh in on the naming controversy?

@mwiebe
Owner

I like having PyArray_SetUpdateIfCopyBase be a public API better than suggesting people use PyArray_BASE, because of its better error checking that caught the nditer bug. In general, I don't really like the UPDATEIFCOPY mechanism, as it relies on Py_DECREF being deterministic to work, and it's easy to imagine something outside of the control of a function using it caching a reference to an array somewhere and causing it to break. I suspect this required special handling for .NET in the numpy-refactor branch? I can understand it needing to stick around for backwards-compatibility reasons, though.

For the renaming of PyArray_RequireWriteable, I slightly favour PyArray_FailUnlessWriteable over PyArray_AssertWriteable, but both of them are ok. Avoiding the "assert" word because it has debug connotations makes sense to me.

@teoliphant
Owner

The UPDATEIFCOPY mechanism was introduced by Numarray and does solve a fundamental problem which you have to solve in another way if you don't do this. The problem is how to allow for an output array that could be arbitrarily mis-behaved while providing to a person wrapping C- or Fortran- code a simple well-behaved array to write to.

I'm not sure why it relies on the determinism of Py_DECREF. The copy-back will occur whenever the deallocation of the shadow occurs. It does rely on not messing with the READONLY flag of the array being shadowed. Of course, if someone keeps a reference to the shadow array, then the copy-back won't ever occur.

I suppose the same functionality could be handled in all cases with PyArray_FromAny and PyArray_CopyInto, though. So, instead of relying on Py_DECREF() of the UPDATEIFCOPY array, you explicitly call PyArray_CopyInto if needed before removing the array. I could see encouraging that style instead of using UPDATEIFCOPY.

Incidentally, deterministic memory management has some benefits (especially for codes that want predictable behavior without wondering whether the garbage collector will suddenly jump in). My enthusiasm for non-deterministic garbage collection strategies is not high. To me, it is a distinct advantage of the CPython run-time.

I like PyArray_FailUnlessWriteable just fine.

@njsmith
Owner

Okay, sounds like we're keeping SetUpdateIfCopyBase, so I "unpushed" that last change to NA_OutputArray (so fa157f2255fee20cccb9a66a250d0c768f7d70fb has disappeared from the pull request list of commits, though github doesn't seem to indicate this anywhere).

That leaves just the debate about AssertWriteable/FailUnlessWriteable/other. It sounds like each has ~3 votes; or maybe something like DemandWriteable or InsistWriteable would be even better. @charris -- thoughts?

@njsmith
Owner

Ping.

@njsmith
Owner

I guess what Chuck is saying here is that he's good with this as it stands? So I'll go ahead and merge this with FailUnlessWriteable, and we can always revisit the issue.

@njsmith njsmith merged commit 51616c9 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 15, 2012
  1. @njsmith

    Consolidate all array writeability checking in new PyArray_RequireWri…

    njsmith authored
    …teable
    
    This is mostly a code cleanup, but it does have a user-visible effect
    in that attempting to write to a unwriteable array now consistently
    raises ValueError. (It used to randomly raise either ValueError or
    RuntimeError.)
    
    Passes numpy.test("full").
  2. @njsmith

    Funnel all assignments to PyArrayObject->base through a single point

    njsmith authored
    This patch removes all direct assignments of non-NULL values to the
    'base' field on PyArrayObjects. A new utility function was created to
    set up UPDATEIFCOPY arrays, and all assignments to 'base' were
    adjusted to use either PyArray_SetBaseObject or the new
    PyArray_SetUpdateIfCopyBase.
    
    One advantage of this is that it produces more consistent error
    handling. This error handling revealed a bug in the nditer code,
    which this patch does *not* yet fix.
    
    The bug is that npyiter_new_temp_array sometimes (when dealing with
    reversed axes) creates an array and then returns a view onto it. But,
    npyiter_allocate_arrays assumes that the array returned by
    npyiter_new_temp_array can have UPDATEIFCOPY set, which requires
    reassigning its 'base' field. Previously, this meant that the
    temporary array leaked. Now, it produces a ValueError.
    
    This code path doesn't seem to actually be hit very often; only one of
    the nditer tests fails because of the change. See
       numpy/core/tests/test_nditer.py:test_iter_array_cast_buggy
Commits on May 16, 2012
  1. @njsmith

    Add a test for ndarray.diagonal()

    njsmith authored
    All 'expected' values were computed using numpy 1.5.1, so we can be
    pretty sure that the recent rewrite didn't change any results.
  2. @njsmith

    Transition scheme for allowing PyArray_Diagonal to return a view

    njsmith authored
    PyArray_Diagonal is changed to return a copy of the diagonal (as in
    numpy 1.6 and earlier), but with a new (hidden) WARN_ON_WRITE flag
    set. Writes to this array (or views thereof) will continue to work as
    normal, but the first write will trigger a DeprecationWarning.
    
    We also issue this warning if someone extracts a non-numpy writeable
    view of the array (e.g., by accessing the Python-level .data
    attribute). There are likely still places where the data buffer is
    exposed that I've missed -- review welcome!
    
    New known-fail test: eye() for maskna arrays was only implemented by
    exploiting ndarray.diagonal's view-ness, so it is now unimplemented
    again, and the corresponding test is marked known-fail.
  3. @njsmith
Commits on May 17, 2012
  1. @njsmith

    Typo fix

    njsmith authored
    Thanks to Travis for catching it.
Commits on May 19, 2012
  1. @njsmith
  2. @njsmith
  3. @njsmith

    Fix a C90 compatibility error

    njsmith authored
  4. @njsmith
Commits on May 21, 2012
  1. @njsmith

    Clean up PyArray_Diagonal changes based on Chuck's feedback

    njsmith authored
    - NPY_ARRAY_WARN_ON_WRITE flag definition is protected by #ifdef
      _MULTIARRAYMODULE, to make totally sure that we can recycle the flag
      bit later.
    - Improve docs for PyArray_SetUpdateIfCopyBase.
    - Make the deprecation warning for writes to the diagonal array
      somewhat more terse.
    - Use DEPRECATE macro instead of calling PyErr_Warn/PyErr_WarnEx
      directly.
    - Comment formatting fixed.
  2. @njsmith
  3. @njsmith
Commits on May 22, 2012
  1. @njsmith
  2. @njsmith

    Rename PyArray_RequireWriteable to PyArray_FailUnlessWriteable

    njsmith authored
    Also clean up its API slightly so that the caller passes in a name
    describing the array being checked which is used to generate an error
    message, rather than writing an error message from scratch.
This page is out of date. Refresh to see the latest.
Showing with 460 additions and 150 deletions.
  1. +43 −24 doc/release/2.0.0-notes.rst
  2. +2 −2 numpy/core/code_generators/cversions.txt
  3. +2 −0  numpy/core/code_generators/numpy_api.py
  4. +21 −1 numpy/core/fromnumeric.py
  5. +5 −1 numpy/core/include/numpy/ndarraytypes.h
  6. +2 −3 numpy/core/numeric.py
  7. +1 −4 numpy/core/src/multiarray/array_assign_array.c
  8. +1 −4 numpy/core/src/multiarray/array_assign_scalar.c
  9. +108 −0 numpy/core/src/multiarray/arrayobject.c
  10. +14 −0 numpy/core/src/multiarray/arrayobject.h
  11. +4 −0 numpy/core/src/multiarray/arraytypes.c.src
  12. +18 −10 numpy/core/src/multiarray/buffer.c
  13. +18 −30 numpy/core/src/multiarray/ctors.c
  14. +14 −1 numpy/core/src/multiarray/getset.c
  15. +14 −5 numpy/core/src/multiarray/item_selection.c
  16. +4 −7 numpy/core/src/multiarray/iterators.c
  17. +2 −6 numpy/core/src/multiarray/mapping.c
  18. +6 −8 numpy/core/src/multiarray/methods.c
  19. +7 −13 numpy/core/src/multiarray/nditer_constr.c
  20. +1 −3 numpy/core/src/multiarray/sequence.c
  21. +5 −6 numpy/core/src/umath/ufunc_object.c
  22. +3 −3 numpy/core/tests/test_maskna.py
  23. +144 −2 numpy/core/tests/test_multiarray.py
  24. +0 −1  numpy/core/tests/test_nditer.py
  25. +12 −3 numpy/lib/twodim_base.py
  26. +9 −13 numpy/numarray/_capi.c
View
67 doc/release/2.0.0-notes.rst
@@ -8,6 +8,41 @@ Highlights
==========
+Compatibility notes
+===================
+
+In a future version of numpy, the functions np.diag, np.diagonal, and
+the diagonal method of ndarrays will return a view onto the original
+array, instead of producing a copy as they do now. This makes a
+difference if you write to the array returned by any of these
+functions. To facilitate this transition, numpy 1.7 produces a
+DeprecationWarning if it detects that you may be attempting to write
+to such an array. See the documentation for np.diagonal for details.
+
+The default casting rule for UFunc out= parameters has been changed from
+'unsafe' to 'same_kind'. Most usages which violate the 'same_kind'
+rule are likely bugs, so this change may expose previously undetected
+errors in projects that depend on NumPy.
+
+Full-array boolean indexing used to allow boolean arrays with a size
+non-broadcastable to the array size. Now it forces this to be broadcastable.
+Since this affects some legacy code, this change will require discussion
+during alpha or early beta testing, and a decision to either keep the
+stricter behavior, or add in a hack to allow the previous behavior to
+work.
+
+Attempting to write to a read-only array (one with
+``arr.flags.writeable`` set to ``False``) used to raise either a
+RuntimeError, ValueError, or TypeError inconsistently, depending on
+which code path was taken. It now consistently raises a ValueError.
+
+The <ufunc>.reduce functions evaluate some reductions in a different
+order than in previous versions of NumPy, generally providing higher
+performance. Because of the nature of floating-point arithmetic, this
+may subtly change some results, just as linking NumPy to a different
+BLAS implementations such as MKL can.
+
+
New features
============
@@ -148,36 +183,20 @@ New argument to searchsorted
The function searchsorted now accepts a 'sorter' argument that is a
permuation array that sorts the array to search.
+C API
+-----
+
+New function ``PyArray_RequireWriteable`` provides a consistent
+interface for checking array writeability -- any C code which works
+with arrays whose WRITEABLE flag is not known to be True a priori,
+should make sure to call this function before writing.
+
Changes
=======
General
-------
-The default casting rule for UFunc out= parameters has been changed from
-'unsafe' to 'same_kind'. Most usages which violate the 'same_kind'
-rule are likely bugs, so this change may expose previously undetected
-errors in projects that depend on NumPy.
-
-Full-array boolean indexing used to allow boolean arrays with a size
-non-broadcastable to the array size. Now it forces this to be broadcastable.
-Since this affects some legacy code, this change will require discussion
-during alpha or early beta testing, and a decision to either keep the
-stricter behavior, or add in a hack to allow the previous behavior to
-work.
-
-The functions np.diag, np.diagonal, and <ndarray>.diagonal now return a
-view into the original array instead of making a copy. This makes these
-functions more consistent with NumPy's general approach of taking views
-where possible, and performs much faster as well. This has the
-potential to break code that assumes a copy is made instead of a view.
-
-The <ufunc>.reduce functions evaluates some reductions in a different
-order than in previous versions of NumPy, generally providing higher
-performance. Because of the nature of floating-point arithmetic, this
-may subtly change some results, just as linking NumPy to a different
-BLAS implementations such as MKL can.
-
The function np.concatenate tries to match the layout of its input
arrays. Previously, the layout did not follow any particular reason,
and depended in an undesirable way on the particular axis chosen for
View
4 numpy/core/code_generators/cversions.txt
@@ -9,5 +9,5 @@
# Version 6 (NumPy 1.6) added new iterator, half float and casting functions,
# PyArray_CountNonzero, PyArray_NewLikeArray and PyArray_MatrixProduct2.
0x00000006 = e61d5dc51fa1c6459328266e215d6987
-# Version 7 (NumPy 1.7) added API for NA, improved datetime64.
-0x00000007 = eb54c77ff4149bab310324cd7c0cb176
+# Version 7 (NumPy 1.7) added API for NA, improved datetime64, misc utilities.
+0x00000007 = 280023b3ecfc2ad0326874917f6f16f9
View
2  numpy/core/code_generators/numpy_api.py
@@ -344,6 +344,8 @@
'NpyNA_FromDTypeAndPayload': 304,
'PyArray_AllowNAConverter': 305,
'PyArray_OutputAllowNAConverter': 306,
+ 'PyArray_FailUnlessWriteable': 307,
+ 'PyArray_SetUpdateIfCopyBase': 308,
}
ufunc_types_api = {
View
22 numpy/core/fromnumeric.py
@@ -932,7 +932,27 @@ def diagonal(a, offset=0, axis1=0, axis2=1):
removing `axis1` and `axis2` and appending an index to the right equal
to the size of the resulting diagonals.
- As of NumPy 1.7, this function always returns a view into `a`.
+ In versions of NumPy prior to 1.7, this function always returned a new,
+ independent array containing a copy of the values in the diagonal.
+
+ In NumPy 1.7, it continues to return a copy of the diagonal, but depending
+ on this fact is deprecated. Writing to the resulting array continues to
+ work as it used to, but a DeprecationWarning will be issued.
+
+ In NumPy 1.8, it will switch to returning a read-only view on the original
+ array. Attempting to write to the resulting array will produce an error.
+
+ In NumPy 1.9, it will still return a view, but this view will no longer be
+ marked read-only. Writing to the returned array will alter your original
+ array as well.
+
+ If you don't write to the array returned by this function, then you can
+ just ignore all of the above.
+
+ If you depend on the current behavior, then we suggest copying the
+ returned array explicitly, i.e., use ``np.diagonal(a).copy()`` instead of
+ just ``np.diagonal(a)``. This will work with both past and future versions
+ of NumPy.
Parameters
----------
View
6 numpy/core/include/numpy/ndarraytypes.h
@@ -917,6 +917,10 @@ typedef int (PyArray_FinalizeFunc)(PyArrayObject *, PyObject *);
*/
#define NPY_ARRAY_ALLOWNA 0x8000
+/*
+ * NOTE: there are also internal flags defined in multiarray/arrayobject.h,
+ * which start at bit 31 and work down.
+ */
#define NPY_ARRAY_BEHAVED (NPY_ARRAY_ALIGNED | \
NPY_ARRAY_WRITEABLE)
@@ -1550,7 +1554,7 @@ static NPY_INLINE PyObject *
PyArray_GETITEM(const PyArrayObject *arr, const char *itemptr)
{
return ((PyArrayObject_fields *)arr)->descr->f->getitem(
- (void *)itemptr, (PyArrayObject *)arr);
+ (void *)itemptr, (PyArrayObject *)arr);
}
static NPY_INLINE int
View
5 numpy/core/numeric.py
@@ -1956,9 +1956,8 @@ def identity(n, dtype=None, maskna=False):
[ 0., 0., 1.]])
"""
- a = zeros((n,n), dtype=dtype, maskna=maskna)
- a.diagonal()[...] = 1
- return a
+ from numpy import eye
+ return eye(n, dtype=dtype, maskna=maskna)
def allclose(a, b, rtol=1.e-5, atol=1.e-8):
"""
View
5 numpy/core/src/multiarray/array_assign_array.c
@@ -449,10 +449,7 @@ PyArray_AssignArray(PyArrayObject *dst, PyArrayObject *src,
return 0;
}
- /* Check that 'dst' is writeable */
- if (!PyArray_ISWRITEABLE(dst)) {
- PyErr_SetString(PyExc_RuntimeError,
- "cannot assign to a read-only array");
+ if (PyArray_FailUnlessWriteable(dst, "assignment destination") < 0) {
goto fail;
}
View
5 numpy/core/src/multiarray/array_assign_scalar.c
@@ -336,10 +336,7 @@ PyArray_AssignRawScalar(PyArrayObject *dst,
int allocated_src_data = 0, dst_has_maskna = PyArray_HASMASKNA(dst);
npy_longlong scalarbuffer[4];
- /* Check that 'dst' is writeable */
- if (!PyArray_ISWRITEABLE(dst)) {
- PyErr_SetString(PyExc_RuntimeError,
- "cannot assign a scalar value to a read-only array");
+ if (PyArray_FailUnlessWriteable(dst, "assignment destination") < 0) {
return -1;
}
View
108 numpy/core/src/multiarray/arrayobject.c
@@ -67,6 +67,57 @@ PyArray_Size(PyObject *op)
}
/*NUMPY_API
+ *
+ * Precondition: 'arr' is a copy of 'base' (though possibly with different
+ * strides, ordering, etc.). This function sets the UPDATEIFCOPY flag and the
+ * ->base pointer on 'arr', so that when 'arr' is destructed, it will copy any
+ * changes back to 'base'.
+ *
+ * Steals a reference to 'base'.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+NPY_NO_EXPORT int
+PyArray_SetUpdateIfCopyBase(PyArrayObject *arr, PyArrayObject *base)
+{
+ if (base == NULL) {
+ PyErr_SetString(PyExc_ValueError,
+ "Cannot UPDATEIFCOPY to NULL array");
+ return -1;
+ }
+ if (PyArray_BASE(arr) != NULL) {
+ PyErr_SetString(PyExc_ValueError,
+ "Cannot set array with existing base to UPDATEIFCOPY");
+ goto fail;
+ }
+ if (PyArray_FailUnlessWriteable(base, "UPDATEIFCOPY base") < 0) {
+ goto fail;
+ }
+
+ /*
+ * Any writes to 'arr' will magicaly turn into writes to 'base', so we
+ * should warn if necessary.
+ */
+ if (PyArray_FLAGS(base) & NPY_ARRAY_WARN_ON_WRITE) {
+ PyArray_ENABLEFLAGS(arr, NPY_ARRAY_WARN_ON_WRITE);
+ }
+
+ /*
+ * Unlike PyArray_SetBaseObject, we do not compress the chain of base
+ * references.
+ */
+ ((PyArrayObject_fields *)arr)->base = (PyObject *)base;
+ PyArray_ENABLEFLAGS(arr, NPY_ARRAY_UPDATEIFCOPY);
+ PyArray_CLEARFLAGS(base, NPY_ARRAY_WRITEABLE);
+
+ return 0;
+
+ fail:
+ Py_DECREF(base);
+ return -1;
+}
+
+/*NUMPY_API
* Sets the 'base' attribute of the array. This steals a reference
* to 'obj'.
*
@@ -104,6 +155,11 @@ PyArray_SetBaseObject(PyArrayObject *arr, PyObject *obj)
PyArrayObject *obj_arr = (PyArrayObject *)obj;
PyObject *tmp;
+ /* Propagate WARN_ON_WRITE through views. */
+ if (PyArray_FLAGS(obj_arr) & NPY_ARRAY_WARN_ON_WRITE) {
+ PyArray_ENABLEFLAGS(arr, NPY_ARRAY_WARN_ON_WRITE);
+ }
+
/* If this array owns its own data, stop collapsing */
if (PyArray_CHKFLAGS(obj_arr, NPY_ARRAY_OWNDATA)) {
break;
@@ -704,6 +760,58 @@ PyArray_CompareString(char *s1, char *s2, size_t len)
}
+/* Call this from contexts where an array might be written to, but we have no
+ * way to tell. (E.g., when converting to a read-write buffer.)
+ */
+NPY_NO_EXPORT int
+array_might_be_written(PyArrayObject *obj)
+{
+ const char *msg =
+ "Numpy has detected that you (may be) writing to an array returned\n"
+ "by numpy.diagonal. This code will likely break in the next numpy\n"
+ "release -- see numpy.diagonal docs for details. The quick fix is\n"
+ "to make an explicit copy (e.g., do arr.diagonal().copy()).";
+ if (PyArray_FLAGS(obj) & NPY_ARRAY_WARN_ON_WRITE) {
+ if (DEPRECATE(msg) < 0) {
+ return -1;
+ }
+ /* Only warn once per array */
+ while (1) {
+ PyArray_CLEARFLAGS(obj, NPY_ARRAY_WARN_ON_WRITE);
+ if (!PyArray_BASE(obj) || !PyArray_Check(PyArray_BASE(obj))) {
+ break;
+ }
+ obj = (PyArrayObject *)PyArray_BASE(obj);
+ }
+ }
+ return 0;
+}
+
+/*NUMPY_API
+ *
+ * This function does nothing if obj is writeable, and raises an exception
+ * (and returns -1) if obj is not writeable. It may also do other
+ * house-keeping, such as issuing warnings on arrays which are transitioning
+ * to become views. Always call this function at some point before writing to
+ * an array.
+ *
+ * 'name' is a name for the array, used to give better error
+ * messages. Something like "assignment destination", "output array", or even
+ * just "array".
+ */
+NPY_NO_EXPORT int
+PyArray_FailUnlessWriteable(PyArrayObject *obj, const char *name)
+{
+ if (!PyArray_ISWRITEABLE(obj)) {
+ PyErr_Format(PyExc_ValueError, "%s is read-only", name);
+ return -1;
+ }
+ if (array_might_be_written(obj) < 0) {
+ return -1;
+ }
+ return 0;
+}
+
/* This also handles possibly mis-aligned data */
/* Compare s1 and s2 which are not necessarily NULL-terminated.
s1 is of length len1
View
14 numpy/core/src/multiarray/arrayobject.h
@@ -12,4 +12,18 @@ _strings_richcompare(PyArrayObject *self, PyArrayObject *other, int cmp_op,
NPY_NO_EXPORT PyObject *
array_richcompare(PyArrayObject *self, PyObject *other, int cmp_op);
+NPY_NO_EXPORT int
+array_might_be_written(PyArrayObject *obj);
+
+/*
+ * This flag is used to mark arrays which we would like to, in the future,
+ * turn into views. It causes a warning to be issued on the first attempt to
+ * write to the array (but the write is allowed to succeed).
+ *
+ * This flag is for internal use only, and may be removed in a future release,
+ * which is why the #define is not exposed to user code. Currently it is set
+ * on arrays returned by ndarray.diagonal.
+ */
+static const int NPY_ARRAY_WARN_ON_WRITE = (1 << 31);
+
#endif
View
4 numpy/core/src/multiarray/arraytypes.c.src
@@ -18,6 +18,7 @@
#include "usertypes.h"
#include "_datetime.h"
#include "na_object.h"
+#include "arrayobject.h"
#include "numpyos.h"
@@ -649,6 +650,9 @@ VOID_getitem(char *ip, PyArrayObject *ap)
* current item a view of it
*/
if (PyArray_ISWRITEABLE(ap)) {
+ if (array_might_be_written(ap) < 0) {
+ return NULL;
+ }
u = (PyArrayObject *)PyBuffer_FromReadWriteMemory(ip, itemsize);
}
else {
View
28 numpy/core/src/multiarray/buffer.c
@@ -13,6 +13,7 @@
#include "buffer.h"
#include "numpyos.h"
+#include "arrayobject.h"
/*************************************************************************
**************** Implement Buffer Protocol ****************************
@@ -57,14 +58,10 @@ array_getreadbuf(PyArrayObject *self, Py_ssize_t segment, void **ptrptr)
static Py_ssize_t
array_getwritebuf(PyArrayObject *self, Py_ssize_t segment, void **ptrptr)
{
- if (PyArray_CHKFLAGS(self, NPY_ARRAY_WRITEABLE)) {
- return array_getreadbuf(self, segment, (void **) ptrptr);
- }
- else {
- PyErr_SetString(PyExc_ValueError, "array cannot be "
- "accessed as a writeable buffer");
+ if (PyArray_FailUnlessWriteable(self, "buffer source array") < 0) {
return -1;
}
+ return array_getreadbuf(self, segment, (void **) ptrptr);
}
static Py_ssize_t
@@ -632,10 +629,21 @@ array_getbuffer(PyObject *obj, Py_buffer *view, int flags)
PyErr_SetString(PyExc_ValueError, "ndarray is not C-contiguous");
goto fail;
}
- if ((flags & PyBUF_WRITEABLE) == PyBUF_WRITEABLE &&
- !PyArray_ISWRITEABLE(self)) {
- PyErr_SetString(PyExc_ValueError, "ndarray is not writeable");
- goto fail;
+ if ((flags & PyBUF_WRITEABLE) == PyBUF_WRITEABLE) {
+ if (PyArray_FailUnlessWriteable(self, "buffer source array") < 0) {
+ goto fail;
+ }
+ }
+ /*
+ * If a read-only buffer is requested on a read-write array, we return a
+ * read-write buffer, which is dubious behavior. But that's why this call
+ * is guarded by PyArray_ISWRITEABLE rather than (flags &
+ * PyBUF_WRITEABLE).
+ */
+ if (PyArray_ISWRITEABLE(self)) {
+ if (array_might_be_written(self) < 0) {
+ goto fail;
+ }
}
if (view == NULL) {
View
48 numpy/core/src/multiarray/ctors.c
@@ -1310,7 +1310,9 @@ _array_from_buffer_3118(PyObject *obj, PyObject **out)
r = PyArray_NewFromDescr(&PyArray_Type, descr,
nd, shape, strides, view->buf,
flags, NULL);
- ((PyArrayObject_fields *)r)->base = memoryview;
+ if (PyArray_SetBaseObject((PyArrayObject *)r, memoryview) < 0) {
+ goto fail;
+ }
PyArray_UpdateFlags((PyArrayObject *)r, NPY_ARRAY_UPDATE_ALL);
*out = r;
@@ -1348,9 +1350,8 @@ PyArray_GetArrayParamsFromObjectEx(PyObject *op,
/* If op is an array */
if (PyArray_Check(op)) {
- if (writeable && !PyArray_ISWRITEABLE((PyArrayObject *)op)) {
- PyErr_SetString(PyExc_RuntimeError,
- "cannot write to array");
+ if (writeable
+ && PyArray_FailUnlessWriteable((PyArrayObject *)op, "array") < 0) {
return -1;
}
Py_INCREF(op);
@@ -1419,9 +1420,8 @@ PyArray_GetArrayParamsFromObjectEx(PyObject *op,
/* If op supports the PEP 3118 buffer interface */
if (!PyBytes_Check(op) && !PyUnicode_Check(op) &&
_array_from_buffer_3118(op, (PyObject **)out_arr) == 0) {
- if (writeable && !PyArray_ISWRITEABLE(*out_arr)) {
- PyErr_SetString(PyExc_RuntimeError,
- "cannot write to PEP 3118 buffer");
+ if (writeable
+ && PyArray_FailUnlessWriteable(*out_arr, "PEP 3118 buffer") < 0) {
Py_DECREF(*out_arr);
return -1;
}
@@ -1440,9 +1440,9 @@ PyArray_GetArrayParamsFromObjectEx(PyObject *op,
}
}
if (tmp != Py_NotImplemented) {
- if (writeable && !PyArray_ISWRITEABLE((PyArrayObject *)tmp)) {
- PyErr_SetString(PyExc_RuntimeError,
- "cannot write to array interface object");
+ if (writeable
+ && PyArray_FailUnlessWriteable((PyArrayObject *)tmp,
+ "array interface object") < 0) {
Py_DECREF(tmp);
return -1;
}
@@ -1462,9 +1462,9 @@ PyArray_GetArrayParamsFromObjectEx(PyObject *op,
if (!writeable) {
tmp = PyArray_FromArrayAttr(op, requested_dtype, context);
if (tmp != Py_NotImplemented) {
- if (writeable && !PyArray_ISWRITEABLE((PyArrayObject *)tmp)) {
- PyErr_SetString(PyExc_RuntimeError,
- "cannot write to array interface object");
+ if (writeable
+ && PyArray_FailUnlessWriteable((PyArrayObject *)tmp,
+ "array interface object") < 0) {
Py_DECREF(tmp);
return -1;
}
@@ -2032,13 +2032,6 @@ PyArray_FromArray(PyArrayObject *arr, PyArray_Descr *newtype, int flags)
order = NPY_CORDER;
}
- if ((flags & NPY_ARRAY_UPDATEIFCOPY) &&
- (!PyArray_ISWRITEABLE(arr))) {
- Py_DECREF(newtype);
- PyErr_SetString(PyExc_ValueError,
- "cannot copy back to a read-only array");
- return NULL;
- }
if ((flags & NPY_ARRAY_ENSUREARRAY)) {
subok = 0;
}
@@ -2078,14 +2071,11 @@ PyArray_FromArray(PyArrayObject *arr, PyArray_Descr *newtype, int flags)
}
if (flags & NPY_ARRAY_UPDATEIFCOPY) {
- /*
- * Don't use PyArray_SetBaseObject, because that compresses
- * the chain of bases.
- */
Py_INCREF(arr);
- ((PyArrayObject_fields *)ret)->base = (PyObject *)arr;
- PyArray_ENABLEFLAGS(ret, NPY_ARRAY_UPDATEIFCOPY);
- PyArray_CLEARFLAGS(arr, NPY_ARRAY_WRITEABLE);
+ if (PyArray_SetUpdateIfCopyBase(ret, arr) < 0) {
+ Py_DECREF(ret);
+ return NULL;
+ }
}
}
/*
@@ -2599,9 +2589,7 @@ PyArray_CopyAsFlat(PyArrayObject *dst, PyArrayObject *src, NPY_ORDER order)
NPY_BEGIN_THREADS_DEF;
- if (!PyArray_ISWRITEABLE(dst)) {
- PyErr_SetString(PyExc_RuntimeError,
- "cannot write to array");
+ if (PyArray_FailUnlessWriteable(dst, "destination array") < 0) {
return -1;
}
View
15 numpy/core/src/multiarray/getset.c
@@ -16,6 +16,7 @@
#include "scalartypes.h"
#include "descriptor.h"
#include "getset.h"
+#include "arrayobject.h"
/******************* array attribute get and set routines ******************/
@@ -260,6 +261,10 @@ array_interface_get(PyArrayObject *self)
return NULL;
}
+ if (array_might_be_written(self) < 0) {
+ return NULL;
+ }
+
/* dataptr */
obj = array_dataptr_get(self);
PyDict_SetItemString(dict, "data", obj);
@@ -355,9 +360,12 @@ array_data_set(PyArrayObject *self, PyObject *op)
PyArray_CLEARFLAGS(self, NPY_ARRAY_UPDATEIFCOPY);
}
Py_DECREF(PyArray_BASE(self));
+ ((PyArrayObject_fields *)self)->base = NULL;
}
Py_INCREF(op);
- ((PyArrayObject_fields *)self)->base = op;
+ if (PyArray_SetBaseObject(self, op) < 0) {
+ return -1;
+ }
((PyArrayObject_fields *)self)->data = buf;
((PyArrayObject_fields *)self)->flags = NPY_ARRAY_CARRAY;
if (!writeable) {
@@ -554,6 +562,11 @@ array_struct_get(PyArrayObject *self)
PyArrayInterface *inter;
PyObject *ret;
+ if (PyArray_ISWRITEABLE(self)) {
+ if (array_might_be_written(self) < 0) {
+ return NULL;
+ }
+ }
inter = (PyArrayInterface *)PyArray_malloc(sizeof(PyArrayInterface));
if (inter==NULL) {
return PyErr_NoMemory();
View
19 numpy/core/src/multiarray/item_selection.c
@@ -1114,9 +1114,7 @@ PyArray_Sort(PyArrayObject *op, int axis, NPY_SORTKIND which)
PyErr_Format(PyExc_ValueError, "axis(=%d) out of bounds", axis);
return -1;
}
- if (!PyArray_ISWRITEABLE(op)) {
- PyErr_SetString(PyExc_RuntimeError,
- "attempted sort on unwriteable array.");
+ if (PyArray_FailUnlessWriteable(op, "sort array") < 0) {
return -1;
}
@@ -1843,7 +1841,11 @@ PyArray_SearchSorted(PyArrayObject *op1, PyObject *op2,
/*NUMPY_API
* Diagonal
*
- * As of NumPy 1.7, this function always returns a view into 'self'.
+ * In NumPy versions prior to 1.7, this function always returned a copy of
+ * the diagonal array. In 1.7, the code has been updated to compute a view
+ * onto 'self', but it still copies this array before returning, as well as
+ * setting the internal WARN_ON_WRITE flag. In a future version, it will
+ * simply return a view onto self.
*/
NPY_NO_EXPORT PyObject *
PyArray_Diagonal(PyArrayObject *self, int offset, int axis1, int axis2)
@@ -1859,6 +1861,7 @@ PyArray_Diagonal(PyArrayObject *self, int offset, int axis1, int axis2)
PyArrayObject *ret;
PyArray_Descr *dtype;
npy_intp ret_shape[NPY_MAXDIMS], ret_strides[NPY_MAXDIMS];
+ PyObject *copy;
if (ndim < 2) {
PyErr_SetString(PyExc_ValueError,
@@ -1989,7 +1992,13 @@ PyArray_Diagonal(PyArrayObject *self, int offset, int axis1, int axis2)
fret->flags |= NPY_ARRAY_MASKNA;
}
- return (PyObject *)ret;
+ /* For backwards compatibility, during the deprecation period: */
+ copy = PyArray_NewCopy(ret, NPY_KEEPORDER);
+ if (!copy) {
+ return NULL;
+ }
+ PyArray_ENABLEFLAGS((PyArrayObject *)copy, NPY_ARRAY_WARN_ON_WRITE);
+ return copy;
}
/*NUMPY_API
View
11 numpy/core/src/multiarray/iterators.c
@@ -1260,14 +1260,11 @@ iter_array(PyArrayIterObject *it, PyObject *NPY_UNUSED(op))
Py_DECREF(ret);
return NULL;
}
- /*
- * Don't use PyArray_SetBaseObject, because that compresses
- * the chain of bases.
- */
Py_INCREF(it->ao);
- ((PyArrayObject_fields *)ret)->base = (PyObject *)it->ao;
- PyArray_ENABLEFLAGS(ret, NPY_ARRAY_UPDATEIFCOPY);
- PyArray_CLEARFLAGS(it->ao, NPY_ARRAY_WRITEABLE);
+ if (PyArray_SetUpdateIfCopyBase(ret, it->ao) < 0) {
+ Py_DECREF(ret);
+ return NULL;
+ }
}
return ret;
View
8 numpy/core/src/multiarray/mapping.c
@@ -176,9 +176,7 @@ array_ass_big_item(PyArrayObject *self, npy_intp i, PyObject *v)
return -1;
}
- if (!PyArray_ISWRITEABLE(self)) {
- PyErr_SetString(PyExc_RuntimeError,
- "array is not writeable");
+ if (PyArray_FailUnlessWriteable(self, "assignment destination") < 0) {
return -1;
}
@@ -1502,9 +1500,7 @@ array_ass_sub(PyArrayObject *self, PyObject *ind, PyObject *op)
"cannot delete array elements");
return -1;
}
- if (!PyArray_ISWRITEABLE(self)) {
- PyErr_SetString(PyExc_RuntimeError,
- "array is not writeable");
+ if (PyArray_FailUnlessWriteable(self, "assignment destination") < 0) {
return -1;
}
View
14 numpy/core/src/multiarray/methods.c
@@ -532,10 +532,7 @@ PyArray_Byteswap(PyArrayObject *self, npy_bool inplace)
copyswapn = PyArray_DESCR(self)->f->copyswapn;
if (inplace) {
- if (!PyArray_ISWRITEABLE(self)) {
- PyErr_SetString(PyExc_RuntimeError,
- "Cannot byte-swap in-place on a " \
- "read-only array");
+ if (PyArray_FailUnlessWriteable(self, "array to be byte-swapped") < 0) {
return NULL;
}
size = PyArray_SIZE(self);
@@ -748,9 +745,7 @@ array_setscalar(PyArrayObject *self, PyObject *args)
"itemset must have at least one argument");
return NULL;
}
- if (!PyArray_ISWRITEABLE(self)) {
- PyErr_SetString(PyExc_RuntimeError,
- "array is not writeable");
+ if (PyArray_FailUnlessWriteable(self, "assignment destination") < 0) {
return NULL;
}
@@ -1697,6 +1692,7 @@ array_setstate(PyArrayObject *self, PyObject *args)
PyArray_CLEARFLAGS(self, NPY_ARRAY_OWNDATA);
}
Py_XDECREF(PyArray_BASE(self));
+ fa->base = NULL;
PyArray_CLEARFLAGS(self, NPY_ARRAY_UPDATEIFCOPY);
@@ -1769,7 +1765,9 @@ array_setstate(PyArrayObject *self, PyObject *args)
Py_DECREF(rawdata);
}
else {
- fa->base = rawdata;
+ if (PyArray_SetBaseObject(self, rawdata) < 0) {
+ return NULL;
+ }
}
}
else {
View
20 numpy/core/src/multiarray/nditer_constr.c
@@ -1098,11 +1098,9 @@ npyiter_prepare_one_operand(PyArrayObject **op,
if (PyArray_Check(*op)) {
npy_uint32 tmp;
- if (((*op_itflags) & NPY_OP_ITFLAG_WRITE) &&
- (!PyArray_CHKFLAGS(*op, NPY_ARRAY_WRITEABLE))) {
- PyErr_SetString(PyExc_ValueError,
- "Operand was a non-writeable array, but "
- "flagged as writeable");
+ if ((*op_itflags) & NPY_OP_ITFLAG_WRITE
+ && PyArray_FailUnlessWriteable(*op, "operand array with iterator "
+ "write flag set") < 0) {
return 0;
}
if (!(flags & NPY_ITER_ZEROSIZE_OK) && PyArray_SIZE(*op) == 0) {
@@ -2984,15 +2982,11 @@ npyiter_allocate_arrays(NpyIter *iter,
}
/* If the data will be written to, set UPDATEIFCOPY */
if (op_itflags[iop] & NPY_OP_ITFLAG_WRITE) {
- /*
- * Don't use PyArray_SetBaseObject, because that compresses
- * the chain of bases.
- */
Py_INCREF(op[iop]);
- ((PyArrayObject_fields *)temp)->base =
- (PyObject *)op[iop];
- PyArray_ENABLEFLAGS(temp, NPY_ARRAY_UPDATEIFCOPY);
- PyArray_CLEARFLAGS(op[iop], NPY_ARRAY_WRITEABLE);
+ if (PyArray_SetUpdateIfCopyBase(temp, op[iop]) < 0) {
+ Py_DECREF(temp);
+ return 0;
+ }
}
Py_DECREF(op[iop]);
View
4 numpy/core/src/multiarray/sequence.c
@@ -119,9 +119,7 @@ array_ass_slice(PyArrayObject *self, Py_ssize_t ilow,
"cannot delete array elements");
return -1;
}
- if (!PyArray_ISWRITEABLE(self)) {
- PyErr_SetString(PyExc_RuntimeError,
- "array is not writeable");
+ if (PyArray_FailUnlessWriteable(self, "assignment destination") < 0) {
return -1;
}
tmp = (PyArrayObject *)array_slice(self, ilow, ihigh);
View
11 numpy/core/src/umath/ufunc_object.c
@@ -795,9 +795,8 @@ static int get_ufunc_arguments(PyUFuncObject *ufunc,
}
/* If it's an array, can use it */
if (PyArray_Check(obj)) {
- if (!PyArray_ISWRITEABLE((PyArrayObject *)obj)) {
- PyErr_SetString(PyExc_ValueError,
- "return array is not writeable");
+ if (PyArray_FailUnlessWriteable((PyArrayObject *)obj,
+ "output array") < 0) {
return -1;
}
Py_INCREF(obj);
@@ -894,9 +893,9 @@ static int get_ufunc_arguments(PyUFuncObject *ufunc,
}
if (PyArray_Check(value)) {
- if (!PyArray_ISWRITEABLE((PyArrayObject *)value)) {
- PyErr_SetString(PyExc_ValueError,
- "return array is not writeable");
+ const char *name = "output array";
+ PyArrayObject *value_arr = (PyArrayObject *)value;
+ if (PyArray_FailUnlessWriteable(value_arr, name) < 0) {
goto fail;
}
Py_INCREF(value);
View
6 numpy/core/tests/test_maskna.py
@@ -1417,11 +1417,9 @@ def test_array_maskna_diagonal():
a.shape = (2,3)
a[0,1] = np.NA
- # Should produce a view into a
res = a.diagonal()
- assert_(res.base is a)
assert_(res.flags.maskna)
- assert_(not res.flags.ownmaskna)
+ assert_(res.flags.ownmaskna)
assert_equal(res, [0, 4])
res = a.diagonal(-1)
@@ -1593,6 +1591,8 @@ def test_array_maskna_linspace_logspace():
assert_(b.flags.maskna)
+from numpy.testing import dec
+@dec.knownfailureif(True, "eye is not implemented for maskna")
def test_array_maskna_eye_identity():
# np.eye
View
146 numpy/core/tests/test_multiarray.py
@@ -29,8 +29,8 @@ def setUp(self):
def test_writeable(self):
mydict = locals()
self.a.flags.writeable = False
- self.assertRaises(RuntimeError, runstring, 'self.a[0] = 3', mydict)
- self.assertRaises(RuntimeError, runstring, 'self.a[0:1].itemset(3)', mydict)
+ self.assertRaises(ValueError, runstring, 'self.a[0] = 3', mydict)
+ self.assertRaises(ValueError, runstring, 'self.a[0:1].itemset(3)', mydict)
self.a.flags.writeable = True
self.a[0] = 5
self.a[0] = 0
@@ -792,6 +792,148 @@ def test_dot(self):
assert_equal(np.dot(a, b), a.dot(b))
assert_equal(np.dot(np.dot(a, b), c), a.dot(b).dot(c))
+ def test_diagonal(self):
+ a = np.arange(12).reshape((3, 4))
+ assert_equal(a.diagonal(), [0, 5, 10])
+ assert_equal(a.diagonal(0), [0, 5, 10])
+ assert_equal(a.diagonal(1), [1, 6, 11])
+ assert_equal(a.diagonal(-1), [4, 9])
+
+ b = np.arange(8).reshape((2, 2, 2))
+ assert_equal(b.diagonal(), [[0, 6], [1, 7]])
+ assert_equal(b.diagonal(0), [[0, 6], [1, 7]])
+ assert_equal(b.diagonal(1), [[2], [3]])
+ assert_equal(b.diagonal(-1), [[4], [5]])
+ assert_raises(ValueError, b.diagonal, axis1=0, axis2=0)
+ assert_equal(b.diagonal(0, 1, 2), [[0, 3], [4, 7]])
+ assert_equal(b.diagonal(0, 0, 1), [[0, 6], [1, 7]])
+ assert_equal(b.diagonal(offset=1, axis1=0, axis2=2), [[1], [3]])
+ # Order of axis argument doesn't matter:
+ assert_equal(b.diagonal(0, 2, 1), [[0, 3], [4, 7]])
+
+ def test_diagonal_deprecation(self):
+ import warnings
+ from numpy.testing.utils import WarningManager
+ def collect_warning_types(f, *args, **kwargs):
+ ctx = WarningManager(record=True)
+ warning_log = ctx.__enter__()
+ warnings.simplefilter("always")
+ try:
+ f(*args, **kwargs)
+ finally:
+ ctx.__exit__()
+ return [w.category for w in warning_log]
+ a = np.arange(9).reshape(3, 3)
+ # All the different functions raise a warning, but not an error, and
+ # 'a' is not modified:
+ assert_equal(collect_warning_types(a.diagonal().__setitem__, 0, 10),
+ [DeprecationWarning])
+ assert_equal(a, np.arange(9).reshape(3, 3))
+ assert_equal(collect_warning_types(np.diagonal(a).__setitem__, 0, 10),
+ [DeprecationWarning])
+ assert_equal(a, np.arange(9).reshape(3, 3))
+ assert_equal(collect_warning_types(np.diag(a).__setitem__, 0, 10),
+ [DeprecationWarning])
+ assert_equal(a, np.arange(9).reshape(3, 3))
+ # Views also warn
+ d = np.diagonal(a)
+ d_view = d.view()
+ assert_equal(collect_warning_types(d_view.__setitem__, 0, 10),
+ [DeprecationWarning])
+ # But the write goes through:
+ assert_equal(d[0], 10)
+ # Only one warning per call to diagonal, though (even if there are
+ # multiple views involved):
+ assert_equal(collect_warning_types(d.__setitem__, 0, 10),
+ [])
+
+ # Other ways of accessing the data also warn:
+ # .data goes via the C buffer API, gives a read-write
+ # buffer/memoryview. We don't warn until tp_getwritebuf is actually
+ # called, which is not until the buffer is written to.
+ have_memoryview = (hasattr(__builtins__, "memoryview")
+ or "memoryview" in __builtins__)
+ def get_data_and_write(getter):
+ buf_or_memoryview = getter(a.diagonal())
+ if (have_memoryview and isinstance(buf_or_memoryview, memoryview)):
+ buf_or_memoryview[0] = np.array(1)
+ else:
+ buf_or_memoryview[0] = "x"
+ assert_equal(collect_warning_types(get_data_and_write,
+ lambda d: d.data),
+ [DeprecationWarning])
+ if hasattr(np, "getbuffer"):
+ assert_equal(collect_warning_types(get_data_and_write,
+ np.getbuffer),
+ [DeprecationWarning])
+ # PEP 3118:
+ if have_memoryview:
+ assert_equal(collect_warning_types(get_data_and_write, memoryview),
+ [DeprecationWarning])
+ # Void dtypes can give us a read-write buffer, but only in Python 2:
+ import sys
+ if sys.version_info[0] < 3:
+ aV = np.empty((3, 3), dtype="V10")
+ assert_equal(collect_warning_types(aV.diagonal().item, 0),
+ [DeprecationWarning])
+ # XX it seems that direct indexing of a void object returns a void
+ # scalar, which ignores not just WARN_ON_WRITE but even WRITEABLE.
+ # i.e. in this:
+ # a = np.empty(10, dtype="V10")
+ # a.flags.writeable = False
+ # buf = a[0].item()
+ # 'buf' ends up as a writeable buffer. I guess no-one actually
+ # uses void types like this though...
+ # __array_interface also lets a data pointer get away from us
+ log = collect_warning_types(getattr, a.diagonal(),
+ "__array_interface__")
+ assert_equal(log, [DeprecationWarning])
+ # ctypeslib goes via __array_interface__:
+ try:
+ # may not exist in python 2.4:
+ import ctypes
+ except ImportError:
+ pass
+ else:
+ log = collect_warning_types(np.ctypeslib.as_ctypes, a.diagonal())
+ assert_equal(log, [DeprecationWarning])
+ # __array_struct__
+ log = collect_warning_types(getattr, a.diagonal(), "__array_struct__")
+ assert_equal(log, [DeprecationWarning])
+
+ # Make sure that our recommendation to silence the warning by copying
+ # the array actually works:
+ diag_copy = a.diagonal().copy()
+ assert_equal(collect_warning_types(diag_copy.__setitem__, 0, 10),
+ [])
+ # There might be people who get a spurious warning because they are
+ # extracting a buffer, but then use that buffer in a read-only
+ # fashion. And they might get cranky at having to create a superfluous
+ # copy just to work around this spurious warning. A reasonable
+ # solution would be for them to mark their usage as read-only, and
+ # thus safe for both past and future PyArray_Diagonal
+ # semantics. So let's make sure that setting the diagonal array to
+ # non-writeable will suppress these warnings:
+ ro_diag = a.diagonal()
+ ro_diag.flags.writeable = False
+ assert_equal(collect_warning_types(getattr, ro_diag, "data"), [])
+ # __array_interface__ has no way to communicate read-onlyness --
+ # effectively all __array_interface__ arrays are assumed to be
+ # writeable :-(
+ # ro_diag = a.diagonal()
+ # ro_diag.flags.writeable = False
+ # assert_equal(collect_warning_types(getattr, ro_diag,
+ # "__array_interface__"), [])
+ if hasattr(__builtins__, "memoryview"):
+ ro_diag = a.diagonal()
+ ro_diag.flags.writeable = False
+ assert_equal(collect_warning_types(memoryview, ro_diag), [])
+ ro_diag = a.diagonal()
+ ro_diag.flags.writeable = False
+ assert_equal(collect_warning_types(getattr, ro_diag,
+ "__array_struct__"), [])
+
+
def test_ravel(self):
a = np.array([[0,1],[2,3]])
assert_equal(a.ravel(), [0,1,2,3])
View
1  numpy/core/tests/test_nditer.py
@@ -865,7 +865,6 @@ def test_iter_array_cast():
i = None
assert_equal(a[2,1,1], -12.5)
- # Unsafe cast 'f4' -> 'i4'
a = np.arange(6, dtype='i4')[::-2]
i = nditer(a, [],
[['writeonly','updateifcopy']],
View
15 numpy/lib/twodim_base.py
@@ -210,19 +210,28 @@ def eye(N, M=None, k=0, dtype=float, maskna=False):
if M is None:
M = N
m = zeros((N, M), dtype=dtype, maskna=maskna)
- diagonal(m, k)[...] = 1
+ if k >= M:
+ return m
+ if k >= 0:
+ i = k
+ else:
+ i = (-k) * M
+ m[:M-k].flat[i::M+1] = 1
return m
def diag(v, k=0):
"""
Extract a diagonal or construct a diagonal array.
- As of NumPy 1.7, extracting a diagonal always returns a view into `v`.
+ See the more detailed documentation for ``numpy.diagonal`` if you use this
+ function to extract a diagonal and wish to write to the resulting array;
+ whether it returns a copy or a view depends on what version of numpy you
+ are using.
Parameters
----------
v : array_like
- If `v` is a 2-D array, return a view of its `k`-th diagonal.
+ If `v` is a 2-D array, return a copy of its `k`-th diagonal.
If `v` is a 1-D array, return a 2-D array with `v` on the `k`-th
diagonal.
k : int, optional
View
22 numpy/numarray/_capi.c
@@ -1077,9 +1077,12 @@ NA_OutputArray(PyObject *a, NumarrayType t, int requires)
PyArray_Descr *dtype;
PyArrayObject *ret;
- if (!PyArray_Check(a) || !PyArray_ISWRITEABLE((PyArrayObject *)a)) {
+ if (!PyArray_Check(a)) {
PyErr_Format(PyExc_TypeError,
- "NA_OutputArray: only writeable arrays work for output.");
+ "NA_OutputArray: only arrays work for output.");
+ return NULL;
+ }
+ if (PyArray_FailUnlessWriteable((PyArrayObject *)a, "output array") < 0) {
return NULL;
}
@@ -1098,12 +1101,10 @@ NA_OutputArray(PyObject *a, NumarrayType t, int requires)
PyArray_DIMS((PyArrayObject *)a),
dtype, 0);
Py_INCREF(a);
- if (PyArray_SetBaseObject(ret, a) < 0) {
+ if (PyArray_SetUpdateIfCopyBase(ret, a) < 0) {
Py_DECREF(ret);
return NULL;
}
- PyArray_ENABLEFLAGS(ret, NPY_ARRAY_UPDATEIFCOPY);
- PyArray_CLEARFLAGS((PyArrayObject *)a, NPY_ARRAY_WRITEABLE);
return ret;
}
@@ -1127,9 +1128,7 @@ NA_IoArray(PyObject *a, NumarrayType t, int requires)
/* Guard against non-writable, but otherwise satisfying requires.
In this case, shadow == a.
*/
- if (!PyArray_ISWRITABLE(shadow)) {
- PyErr_Format(PyExc_TypeError,
- "NA_IoArray: I/O array must be writable array");
+ if (!PyArray_FailUnlessWriteable(shadow, "input/output array")) {
PyArray_XDECREF_ERR(shadow);
return NULL;
}
@@ -2488,13 +2487,10 @@ _setFromPythonScalarCore(PyArrayObject *a, long offset, PyObject*value, int entr
static int
NA_setFromPythonScalar(PyArrayObject *a, long offset, PyObject *value)
{
- if (PyArray_FLAGS(a) & NPY_ARRAY_WRITEABLE)
- return _setFromPythonScalarCore(a, offset, value, 0);
- else {
- PyErr_Format(
- PyExc_ValueError, "NA_setFromPythonScalar: assigment to readonly array buffer");
+ if (PyArray_FailUnlessWriteable(a, "array") < 0) {
return -1;
}
+ return _setFromPythonScalarCore(a, offset, value, 0);
}
Something went wrong with that request. Please try again.