SubArray of subArray now returns subArray of original parent #3538
Conversation
src/backend/cpu/Array.cpp
Outdated
| // (sub)Arrays remain linear when the strides corresponds to dataStrides | ||
| bool parent_isLinear = (parent_strides[0] == 1); | ||
| for (dim_t i = parent.ndims() - 1; i > 0; i--) { | ||
| parent_isLinear &= |
There was a problem hiding this comment.
This check for linear solves the issue. Is the currently isLinear member function not correct?
There was a problem hiding this comment.
The loop is not a linear check on the parent array. It is a linear check on the data written in the buffer.
For a subarray of an array, the parent (dims & strides) or equal to those in the data array.
For a subarray of a subarray, the parent is already a subarray and therefor likely non-linear. However as long as the data array is linear, the operation is still possible.
Difference between code isLinear() and this:
- isLinear(): Dims and Strides are from array
- this code: Dims are from data array and Strides are from array
I will update the comments to clarify this further, and I will change the code so that it resembles more on the isLinear() code. The difference will be noticed easier.
|
Works, tested on Windows and Ubuntu 22.04 on all backends |
|
The PR is rebased on the last commit in master and force pushed. The 2 previous requested changes are applied. |
|
It looks like this issue only occurs in the special case where a subarray is assigned to the original variable for the parent array. In other cases the reference count will be greater than 1 and a copy is made in af_assign_gen/af_assign_seq. While this fix works for this particular test I think it would be safer to add a linear check earlier in src/api/c/assign.cpp and make a copy there before the createSubArray call. What do you think @umar456 ? |
|
The cause of the original bug, was the fact that a copy was taken, because the first subArray was considered non-linear. This resulted in assign the rhs to the copy, which is discarded in the end. The original parent remained unchanged. Making a copy a level higher and building a subArray on the copy will have the same result as the original code. |
It is discarded because the parent argument to createSubArray is a const reference and is left unchanged when it returns. That is why I suggested a linear check before the function call, see: https://github.com/christophe-murphy/arrayfire/tree/alternative_fix_issue_3534 |
|
I think it is safer to use the fix I suggested instead ^ however if we are going to use this fix we will need more tests to make sure it handles all possible sub-arrays properly. All combinations of indexing by sequence, span, and by an arbitrary array of non-contiguous indices along each dimension should be tried to make sure that the sub-array is created correctly. |
…rrently there are failures for all back ends except for the CPU back end. This appears to be because it has been assumed that the array dims are equivalent to the data_dims which is not necessarily the case when we have a sub-array.
|
I have extended the testing. Right now there are failures on all the back ends except the CPU back end. I'm looking into why but it seems to be because we assume that dims is equivalent to data_dims in the assign kernels. Not sure if this is an oversight or intentional. |
…is has fixed the failures in the tests for assigning to a sub-array of a sub-array. All tests in test/array.cpp are passing.
| // We expect a copy to be created, so the handle and | ||
| // array pointers should be different | ||
| ASSERT_NE(before_handle, after_handle); | ||
| ASSERT_NE(before_dev_pointer, after_dev_pointer); |
There was a problem hiding this comment.
There is a potential for these pointers to be the same because the device pointer is linear even though its calling a sub array. This is not the case for the rows based tests above because the subarray would not be linear.
left-hand subArrays always return the parent Array with a new dimensions/strides (copy parameter == false).
When the parent Array however already was an subArray, an internal copy was executed and a subArray on the internal copy was returned. All updates performed on this left-hand subArray were lost.
Reason:
the subArray calculation assumes that the parent array is linear. If not, a copy is created before starting the calculation.
For left-hand subArrays, a copy is not allowed because we would no longer write into the parent data buffer. It is also not necessary for parent subArrays since the original parent array still is linear (although the parent subArray is not).
Solution:
The linear check is changed so that we now verify the original parent array on linearity. In case of non-linear a copy is still performed when the copy parameter is true, otherwise an AF_ERROR is thrown.
Description
Additional information about the PR answering following questions:
-->
Fixes: [BUG] Elementwise value assignment silently fails after in-place array downsize using
rows()#3534Changes to Users
No changes
Checklist