Fix memory continuity judgment when stride is negative #885

SparrowLii · 2021-01-06T08:42:23Z

It comes from FIXME in is_cotiguous. Whether the stride is negative will not affect the arrangement of the array in the memory, therefore, sorting and continuity detection can be performed according to the absolute value of the step size. That's what this pr dose in dimension_trait.rs.

Then we need to deal with the as_slice_memory_order function in impl_methods.rs. When stride is negative, although we can know that the array is continuous, because the ptr of the array does not point to the head of the memory data at this time, the ptr value cannot be used directly.

This is why I added the head_ptr_offset function in dimension/mod.rs. It simply calculates the offset of the head address of the array data relative to the current ptr based on the dim and stride of the array. The offset is always less than or equal to 0. After calculating the offset, as_slice_memory_order can get continuous data. as_slice_memory_order_mut is the same.

The last thing to be solved is the to_owned function in impl_method.rs. After calling as_slice_memory_order, it will call from_shape_vec_unchecked function( in impl_constructors.rs ) to generate a new ArrayBase based on the obtained array head pointer. This is incorrect when stride is negative, because the head address of the data array is not the ptr value that ArrayBase should have. Fortunately, we have the head_ptr_offset method, which can simply calculate the offset based on dim and strides, and then get the ptr we need.

src/impl_methods.rs

src/dimension/mod.rs

src/impl_constructors.rs

bluss

Thanks a lot for working on this, it would be awesome to get this fixed!

src/dimension/mod.rs

src/dimension/dimension_trait.rs

SparrowLii · 2021-01-07T12:29:01Z

@bluss This line is a bit strange( tests/oper.rs:775 )
It runs &lhs.to_owned().into_shape(), but to_owned() does not guarantee that the memory arrangement of the return value is standard C or fortran( which into_shape() needs), regardless of whether the stride is negative.
This is the reason why my test failed, I will try to modify it, but I hope you can provide some suggestions.

bluss

Looks good. With a few minor changes - and tests as mentioned in other comments - I think this could be ready for merge.

@jturner314 Any comments on the approach here?

src/dimension/mod.rs

src/impl_constructors.rs

bluss · 2021-01-07T19:13:52Z

tests/oper.rs

@@ -707,7 +707,7 @@ fn gen_mat_vec_mul() {
        S2: Data<Elem = A>,
    {
        let ((m, _), k) = (lhs.dim(), rhs.dim());
-        reference_mat_mul(lhs, &rhs.to_owned().into_shape((k, 1)).unwrap())
+        reference_mat_mul(lhs, &rhs.as_standard_layout().into_shape((k, 1)).unwrap())


Thanks, this change makes sense - it is a sign of .to_owned() improving and becoming more capable which is great. But we'll have to make a compatibility note or something.

I added relevant instructions in the comment of to_owned. Maybe we should clarify the role of the return value of to_owned and improve it?

to_owned's docs are fine - it's into_shape that's a bit tricky to use, and to_owned doesn't need to account for that 🙂

jturner314

I'm glad this will finally be addressed.

This PR makes it possible to directly construct arrays with negative strides using from_shape_vec, but it doesn't address the awkwardness of doing this using the existing API (since the existing API requires casting isize to usize in a confusing way), but that's okay. We could document how to construct an array with negative strides using from_shape_vec, but due to the awkwardness, IMO it's fine to leave it undocumented until we have a better API for this.

I haven't reviewed the PR in detail, but the general approach looks fine. I would like to see a few tests for the constructors with negative strides.

I commented on changes that need to be made to the can_index_slice docs (see below). Personally, I'd prefer for can_index_slice to take as a parameter either the offset to the first element or the pointer to the first element, instead of just adding a warning to the docs of how the pointer needs to be computed. (I think adding it as an explicit parameter would make accidentally misusing the function less likely.) That said, the current implementation is fine as long as the warning is added.

src/dimension/mod.rs

SparrowLii · 2021-01-08T14:56:34Z

@jturner314 I added the corresponding warning. I think it is a matter to construct an array based on the negative step size, and there are related issues in ndarray. I can fix them in the next pr. WDYT?

SparrowLii · 2021-01-08T15:06:45Z

I added the corresponding test in tests/array.rs, It should work well now.

bluss · 2021-01-08T11:55:40Z

src/impl_methods.rs

-    /// can allocate a new array with the desired memory layout and
-    /// [`.assign()`](#method.assign) the data. Alternatively, you can collect
-    /// an iterator, like this for a result in standard layout:
+    /// If the input array is contiguous, then theoutput array will have the same


The wrapping lost some spaces here: theoutput and so on

Also, I don't want to guarantee that: "Otherwise, the layout of the output array is standard C." I'd much rather leave the sentence as-is: "Otherwise, the layout of the output array is unspecified." In the future, we may change .to_owned() to more intelligently choose the layout of the output array for the non-contiguous-input cases.

src/impl_methods.rs

src/dimension/mod.rs

bluss · 2021-01-08T19:30:58Z

tests/oper.rs

@@ -707,7 +707,7 @@ fn gen_mat_vec_mul() {
        S2: Data<Elem = A>,
    {
        let ((m, _), k) = (lhs.dim(), rhs.dim());
-        reference_mat_mul(lhs, &rhs.to_owned().into_shape((k, 1)).unwrap())
+        reference_mat_mul(lhs, &rhs.as_standard_layout().into_shape((k, 1)).unwrap())


to_owned's docs are fine - it's into_shape that's a bit tricky to use, and to_owned doesn't need to account for that 🙂

bluss · 2021-01-09T09:36:54Z

src/impl_methods.rs

-    /// If the input array is contiguous, then theoutput array will have the same
-    /// memory layout. Otherwise, the layout ofthe output array is standard C.
+    /// If the input array is contiguous, then the output array will have the same
+    /// memory layout. Otherwise, the layout ofthe output array is unspecified.


Suggested change

/// memory layout. Otherwise, the layout ofthe output array is unspecified.

/// memory layout. Otherwise, the layout of the output array is unspecified.

And so on, there's a missing space on every line

So sorry for that missing

No problem. It's a good PR if both the maintainer and contributor have time and patience for reviewing in several cycles. Even though it of course feels awkward to request more changes etc. For an open source project I think on balance it makes sense to require high quality - contributors are in general not known if they will stay around and tie up loose ends and we avoid merging half-baked features. And merging PRs with too many loose ends that the maintainer has to fix - that burns out the maintainer. That's the sum of my experience.

Thank you very much, it means a lot to me.

Changes done

bluss · 2021-01-10T13:38:57Z

This ready to merge IMO.

You have fixed is_contiguous and the many methods that rely on it, and that's great, I think that's why the maintainers are excited about this PR.

We have left the interface unspecified in the current state of the PR - w.r.t creating array views and raw views with negative strides. For the user, it's not clear if it's allowed and how to specify the pointer - should it be the first-in-memory or the first-in-array pointer that they should pass?

For this reason, I think all the public from-raw-pointer constructors for array views and raw views must have a debug assertion that denies negative strides, and a method doc that states that strides cannot be negative - the reason is that this must be a closed possibility until the interface is decided. This change should not affect our own functionality - we internally use private constructors for array views (that always use the first-in-the-array pointer).

I can do that change that closes down the negative strides in those constructors - if you agree.

jturner314 · 2021-01-10T23:23:26Z

For this reason, I think all the public from-raw-pointer constructors for array views and raw views must have a debug assertion that denies negative strides, and a method doc that states that strides cannot be negative - the reason is that this must be a closed possibility until the interface is decided.

I agree.

We could also do this for the public owned constructors such as from_shape_vec_unchecked, if there's a possibility that we'll add a way for the user to specify the offset to the first element instead of just assuming that the lowest-memory-address element should be at the start of the Vec. (We'd need make sure to change to_owned, map, and map_mut to use from_vec_dim_stride_unchecked instead of from_shape_vec_unchecked so that they'd still work for the "contiguous with negative strides" case.)

SparrowLii · 2021-01-11T01:26:17Z

@bluss I very much agree. The description of the functions using ptr does not seem to take into account the difference between memory pointers and vector pointers in negative strides. I think that the names and documents of these APIs (and perhaps implementations) should be modified to clarify the definition and use of parameters, like @jturner314 pointed out.

I think this pr can be merged, and thank you very much for your reviewing and help.

bluss · 2021-01-11T19:42:44Z

@jturner314 That's an interesting and complicating point. I didn't think of that - with the constructors that start from a Vec, it is already clear where the start of the memory is, I was thinking, so it's not strictly necessary to allow the user to specify a different offset. And to_owned and map already handle every applicable case right with this PR if I read it correctly.

jturner314 · 2021-01-11T21:19:08Z

with the constructors that start from a Vec, it is already clear where the start of the memory is, I was thinking, so it's not strictly necessary to allow the user to specify a different offset.

I agree that the most obvious use-case is when the lowest-address element corresponds to the start of the owned data. However, once we allow negative strides in the constructors, it'll be possible for users to directly construct everything ArrayBase can represent except for the case where the lowest-address element is not at the start of the owned data. While I don't immediately see an obvious use-case for directly constructing an owned array like this, it would be nice to support it in the constructors because the alternative ways to construct an array like this would be painful. (You'd have to create an Array1 and then perform a complicated series of slicing/reshaping operations. Alternatively, you could drop the elements from the start of the Vec that you don't want so that the lowest-address element would be at the start of the Vec, but AFAIK, there's not a cheap way to do that.)

to_owned and map already handle every applicable case right with this PR if I read it correctly.

That's correct. We'd only need to change them if we added debug assertions for negative strides to from_shape_vec_unchecked (since they use from_shape_vec_unchecked with strides that can be negative).

bluss · 2021-01-11T22:59:55Z

Without a use case, we can decline implementing it 🙂

jturner314 · 2021-01-11T23:28:04Z

That would be fine with me; I'd just like to keep this possible functionality in mind while updating the constructor API for negative strides, so that we don't accidentally make it too difficult to add in the future (in case this functionality is desirable for someone). If we do add this functionality in the future, I'd make it optional, similar to how .strides() is currently handled; the default would be to place the lowest-address element at the start of the owned data.

jturner314 · 2021-01-12T00:15:45Z

I just thought of one example where it could be useful -- if you wanted an alignment greater than the alignment of the element type, e.g. for SIMD. (A common way to do this is to allocate a larger Vec than needed and ignore the first few elements.)

SparrowLii · 2021-01-12T02:11:20Z

I think we should update the description of these constructors, but there is no need to add an offset parameter. As long as the meaning of the ptr parameter in the API is clear (is it a memory head pointer or an array head pointer), we can construct it correctly. Even if we update functions constructed array with negative strides in the future.

For the support of simd, I think we should implement a new data structure for it. Then we can add the offset parameter to it.

bluss · 2021-01-13T20:17:08Z

@jturner314 That sounds intriguing, since it can be supported without any extra flags or carrying extra alignment info inside Array<A, D>. But it can have its own constructor I think

…ation

jturner314 · 2021-01-13T21:31:59Z

@jturner314 That sounds intriguing, since it can be supported without any extra flags or carrying extra alignment info inside Array<A, D>. But it can have its own constructor I think

I agree, it's not a strong justification for an offset parameter.

One other reason it could be useful for owned constructors to support an offset parameter is for testing. (Ideally, it would be easy to write tests where the lowest-address element doesn't match the start of the owned data for arrays with arbitrary strides. While most user code shouldn't care about how the array maps to the underlying storage, it may be relevant for code which converts back-and-forth from Vec using into_raw_vec, or code involving unsafe.)

bluss · 2021-01-13T22:06:50Z

Rebased to squash together to more concise history

For raw views and array views, require non-negative strides and check this with a debug assertion.

bluss · 2021-01-13T23:26:02Z

Only the raw pointer constructors deny constructing with negative strides. The vec ones simply leave it unspecified, and it's not intended to be supported - pending design. Thanks for fixing this long-standing fixme, the library is now much better.