{"action":"created","issue":{"url":"https://api.github.com/repos/numpy/numpy/issues/6053","repository_url":"https://api.github.com/repos/numpy/numpy","labels_url":"https://api.github.com/repos/numpy/numpy/issues/6053/labels{/name}","comments_url":"https://api.github.com/repos/numpy/numpy/issues/6053/comments","events_url":"https://api.github.com/repos/numpy/numpy/issues/6053/events","html_url":"https://github.com/numpy/numpy/pull/6053","id":93831872,"number":6053,"title":"MAINT: struct assignment \"by field position\", multi-field indices return views","user":{"login":"ahaldane","id":9040124,"avatar_url":"https://avatars.githubusercontent.com/u/9040124?v=3","gravatar_id":"","url":"https://api.github.com/users/ahaldane","html_url":"https://github.com/ahaldane","followers_url":"https://api.github.com/users/ahaldane/followers","following_url":"https://api.github.com/users/ahaldane/following{/other_user}","gists_url":"https://api.github.com/users/ahaldane/gists{/gist_id}","starred_url":"https://api.github.com/users/ahaldane/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ahaldane/subscriptions","organizations_url":"https://api.github.com/users/ahaldane/orgs","repos_url":"https://api.github.com/users/ahaldane/repos","events_url":"https://api.github.com/users/ahaldane/events{/privacy}","received_events_url":"https://api.github.com/users/ahaldane/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/numpy/numpy/labels/10%20-%20Maintenance","name":"10 - Maintenance","color":"fef2c0"},{"url":"https://api.github.com/repos/numpy/numpy/labels/component:%20numpy.core","name":"component: numpy.core","color":"009800"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/numpy/numpy/milestones/41","html_url":"https://github.com/numpy/numpy/milestones/1.12.0%20release","labels_url":"https://api.github.com/repos/numpy/numpy/milestones/41/labels","id":1503092,"number":41,"title":"1.12.0 release","description":"Tag for the Numpy 1.12.0 release.","creator":{"login":"charris","id":77272,"avatar_url":"https://avatars.githubusercontent.com/u/77272?v=3","gravatar_id":"","url":"https://api.github.com/users/charris","html_url":"https://github.com/charris","followers_url":"https://api.github.com/users/charris/followers","following_url":"https://api.github.com/users/charris/following{/other_user}","gists_url":"https://api.github.com/users/charris/gists{/gist_id}","starred_url":"https://api.github.com/users/charris/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/charris/subscriptions","organizations_url":"https://api.github.com/users/charris/orgs","repos_url":"https://api.github.com/users/charris/repos","events_url":"https://api.github.com/users/charris/events{/privacy}","received_events_url":"https://api.github.com/users/charris/received_events","type":"User","site_admin":false},"open_issues":50,"closed_issues":23,"state":"open","created_at":"2016-01-12T20:39:09Z","updated_at":"2016-06-06T17:38:54Z","due_on":null,"closed_at":null},"comments":6,"created_at":"2015-07-08T16:20:13Z","updated_at":"2016-06-06T17:42:19Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/numpy/numpy/pulls/6053","html_url":"https://github.com/numpy/numpy/pull/6053","diff_url":"https://github.com/numpy/numpy/pull/6053.diff","patch_url":"https://github.com/numpy/numpy/pull/6053.patch"},"body":"This is a try at improving structured array assignment, aiming for 1.11. The goal is to remove various inconsistencies in structure assignment, and to allow multi-field indexing to return a view.\r\n\r\nThe biggest change is that assignment from one structured array to another now works \"by position\" instead of \"by field name\", as in this example:\r\n\r\n    >>> arr1 = np.array([(1,2,3)], dtype=[('a', 'i4'), ('b', 'i4'), ('c', 'i4')])\r\n    >>> arr2 = np.array([(4,5,6)], dtype=[('b', 'i4'), ('a', 'i4'), ('c', 'i4')])\r\n    >>> arr1[:] = arr2\r\n\r\nPreviously, arr1 would be set to \"(5,4,6)\" but with this PR it is set to \"(4,5,6)\". \r\n\r\nThis fixes the issues in structured array assignment outlined below, and allows casting/assignment in new situations. Then I allow multi-field indexing (like `arr1[['b', 'c']]`) to return a view instead of a copy (issue #5994), since assignment to this view will now work more sensibly.\r\n\r\nThis PR also fixes #2353 #6085 #3351.\r\n\r\n### Problems in structure assignment\r\n\r\nThere are two code paths used to assign structured items, 1. using `setitem` or 2. using the `dtype_transfer` functions. The two paths behave differently from each other, and each sometimes have unexpected behaviors. Consider this setup:\r\n\r\n    def testassign(arr, v):\r\n        c = arr.copy()\r\n        c[0] = v  # assign using setitem\r\n        c[1:] = v # assign using \"dtype_transfer\"\r\n        print(c)\r\n\r\n    arr = ones(2, dtype=[('foo', 'i8'), ('bar', 'i8')])\r\n    v1 = array([(2,3)], dtype=[('foo', 'i8'), ('bar', 'i8')])\r\n    v2 = array([(2,3)], dtype=[('bar', 'i8'), ('foo', 'i8')])\r\n    v3 = array([(2,3)], dtype=[('bar', 'i8'), ('baz', 'i8')])\r\n    v4 = array([(2,)],  dtype=[('bar', 'i8')])\r\n    v5 = array([(2,3)], dtype=[('foo', 'f8'), ('bar', 'f8')])\r\n    w = arr.view({'names': ['bar'], 'formats': ['i8'], 'offsets': [8]})\r\n\r\n    testassign(arr, v1)\r\n    testassign(arr, v2)\r\n    testassign(arr, v3)\r\n    testassign(arr, v4)\r\n    testassign(arr, v5)\r\n    w[:] = 4\r\n    print arr\r\n\r\nOutput:\r\n\r\n    [(2, 3) (2, 3)]\r\n    [(2, 3) (3, 2)]\r\n    [(2, 3) (0, 2)]\r\n    [(2, 0) (0, 2)]\r\n    [(4607182418800017408, 4611686018427387904) (2, 3)]\r\n    [(140514438937232, 4) (140514438937232, 4)]\r\n\r\nIn summary, only the assignment using exactly the same dtype (v1) works, all other cases are broken in some way. For v2, `setitem` effectively assigns the fields \"by position\", but `dtype_transfer` code assigns \"by field name\". For v3, `dtype_transfer` converts \"missing\" fields to 0. In v4 `dtype_transfer` sets missing fields to 0, while `setitem` pads short buffers with 0. For v5, `setitem` copies the bytes directly (no casting). For the \"partial view\" `w`, we get uninitialized values at unviewed fields in both paths.\r\n\r\n### Proposed Solution\r\n\r\nIn this PR I made all field assignment occur \"by position\", and cast if needed. That is, if you order the fields from left to right in dst and src arrays, dst field 0 gets set to src field 0 and so on, regardless of field name. This makes sense since the \"python representation\" of a structured element (in `VOID_getitem`) is a tuple, and tuple assignment to a structure works \"by position\" too:\r\n\r\n    >>> a[0] = (1,2)\r\n    >>> a[0] = a[1].item() #works the same, essentially \"a.setitem(0, a.getitem(1))\"\r\n\r\nAssignment to a different number of fields (v4 above) is no longer allowed. With this behavior, the output of the example will be:\r\n\r\n    [(2, 3) (2, 3)]\r\n    [(2, 3) (2, 3)]\r\n    [(2, 3) (2, 3)]\r\n    # v4 raises exception\r\n    [(2, 3) (2, 3)]\r\n    [(1, 4) (1, 4)]\r\n\r\nThis is a backward incompatible change, but I suspect it's not a big problem for most people given how inconsistently structure assignment worked. The most common case - assigning using an identical dtype (v1) - behaves the same as before. I ran unit tests for scipy, pandas and astropy with the PR on my system (python2, x64 linux). Scipy and pandas are unaffected, and astropy fails 1 new test ([this one](https://github.com/astropy/astropy/blob/master/astropy/units/tests/test_quantity_array_methods.py#L557)). \r\n\r\nOne alternative is to make all assignment work \"by field name\", but that also involves a compatibility break for all of the setitem cases. The fact that `dtype_transfer` code assigns by field name is currently undocumented (as far as I see), however there are a couple unit tests that test whether \"reordering\" of fields is allowed (like in v2), which depends on assignment by field name. I removed/modified these tests. I think assignment by field name is confusing and bug prone though, and without it I was able to get rid of a number of unit tests for edge cases we no longer have to worry about (eg, checking that \"unviewed\" fields of various kinds got set to 0 or None).\r\n\r\nHistorical note: Based on some comments in unit tests it looks like numpy 1.5 and before assigned fields \"by position\", but this was changed in 1.6 to assignment \"by field name\" in PR #36, though this was not noted anywhere in docs or release notes.\r\n\r\nImplementation involved rewriting parts of `VOID_setitem` and `get_fields_transfer_function`. \r\n\r\n### Multi-Field Assignment\r\n\r\nWith these changes it is now reasonable to allow writeable multi-field views, which allows assignment like:\r\n\r\n    >>> a = np.ones(4, dtype=[('foo', 'i8'), ('bar', 'i8'), ('baz', 'f4')])\r\n    >>> a[['bar', 'baz']] = (2, 3.0)\r\n    >>> a[['baz', 'foo']][:2] = (4.0, 5)\r\n    >>> a\r\n    array([(5, 2, 4.0), (5, 2, 4.0), (1, 2, 3.0), (1, 2, 3.0)], \r\n          dtype=[('foo', '<i8'), ('bar', '<i8'), ('baz', '<f4')])\r\n\r\nNote that with assignment \"by position\" reordering the fields (like in v2) would no longer work. However, multi-field assignment makes up for this, since fields will be assigned in the order they are subscripted, allowing reordering in a different way. For example, \r\n\r\n    >>> a[['foo', 'bar']] = a[['bar', 'foo']]\r\n\r\nswaps the field data (while it was a no-op/broken in the old assignment \"by field name\").\r\n\r\nNote that multi-field views (whether by this PR or another) introduce another backward incompatible change, since the byte-arrangement of a multi-field copy is different from the byte-arrangement of a multi-field view:\r\n\r\n    >>> a = ones(10, dtype='i8,i8,i8')\r\n    >>> a[['f0','f2']].dtype\r\n    # old ouput, for the multi-field copy:\r\n    dtype([('f0', '<i8'), ('f2', '<i8')])\r\n    # this PR's output, for multi-field view\r\n    dtype({'names':['f0','f2'], 'formats':['<i8','<i8'], 'offsets':[0,16], 'itemsize':24})\r\n\r\nThis is a problem for code which does things like `a[['f0','f2']].view('i4')` (like in unit test `test_field_names`), which would end up viewing different values in the old vs new code. In the new code this particular view raises an error as a result of my other PR #4488 to add safety checks to views. Similarly, `np.recarray` code uses such a view internally, and needed to be fixed. There might be a similar problem for the planned conversion to `np.diagonal` to a view.\r\n\r\nI suggest we add a deprecation warning for code that attempts such a view in 1.10. Multi-field views have been planned for a long time and there is already a deprecation warning that copies will become views, but the warning only appears if you try to assign to the copy, and not if you try to view the copy. I've made another PR, #6054, which adds the appropriate deprecation warnings.\r\n\r\nThis change required a small edit to `_internals.py`, and removing warnings in `mapping.c`.\r\n\r\n### Other changes\r\n\r\n1) One difference between `setitem` and `dtype_transfer` was that `dtype_transfer` allows assignment of a scalar to a structure (`a[ind] = 1`), which sets all fields to that value, while `setitem` did not. This is important for eg `np.ones`, which initializes the array with such a statement. I made it so both paths now allow assignment using a scalar.\r\n\r\n2) Relatedly, got rid of undocumented special case for assignment from a structure to a scalar array of boolean type: If the scalar was of boolean type it would be set to \"True\" regardless of the values in the structured array, with the explanation that \"the existence of fields implies True\". This broke a MaskedArray test, which I fixed by updating `make_mask` to do the equivalent.\r\n\r\n3) Previously, if you tried to assign a structured array to a non-structured array just the first field was copied over. Eg\r\n\r\n     >>> a = np.array([1,2,3])\r\n     >>> b = np.array([(10,20,3.0)], dtype='i4,i8,f4')\r\n     >>> a[:] = b\r\n     >>> a\r\n     array([10, 10, 10])\r\n\r\nNow, I made it raise an exception unless the structured type has only one field, since I found it confusing.\r\n\r\n4) Slightly changed PyArray_EquivTypes for structured arrays. Now it checks that the fields are in the same order in both dtypes. (previously only cared that the dtype.fields dicts had the same values). In other words, field order is now important!\r\n\r\n5) I rewrote most of the docs on structured arrays. A possibly iffy part of the new docs is the section \"obsolete features\", in which I discourage use of field titles and use the \"second\" dictionary based form of dtype specification.\r\n"},"comment":{"url":"https://api.github.com/repos/numpy/numpy/issues/comments/224032382","html_url":"https://github.com/numpy/numpy/pull/6053#issuecomment-224032382","issue_url":"https://api.github.com/repos/numpy/numpy/issues/6053","id":224032382,"user":{"login":"charris","id":77272,"avatar_url":"https://avatars.githubusercontent.com/u/77272?v=3","gravatar_id":"","url":"https://api.github.com/users/charris","html_url":"https://github.com/charris","followers_url":"https://api.github.com/users/charris/followers","following_url":"https://api.github.com/users/charris/following{/other_user}","gists_url":"https://api.github.com/users/charris/gists{/gist_id}","starred_url":"https://api.github.com/users/charris/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/charris/subscriptions","organizations_url":"https://api.github.com/users/charris/orgs","repos_url":"https://api.github.com/users/charris/repos","events_url":"https://api.github.com/users/charris/events{/privacy}","received_events_url":"https://api.github.com/users/charris/received_events","type":"User","site_admin":false},"created_at":"2016-06-06T17:42:19Z","updated_at":"2016-06-06T17:42:19Z","body":"@ahaldane probably need to look at getting this into 1.12. Could you take care of the rebase and take a rested look at the code?"}}
{"actor":{"display_login":"charris"}}