Some follow-up work for#885, but also some preparation work for#842.
Although we currently do not support the use of negative strides to build arrays, there may be users who write like this:

let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13];let mut a= Array::from_shape_vec((2, 3, 2).strides(((-1isize as usize), 4, 2)),s[0..12].to_vec()).unwrap();println!("{:?}", a);

Bacuase of#885, this is just all right:

[[[1, 3],  [5, 7],  [9, 11]], [[0, 2],  [4, 6],  [8, 10]]], shape=[2, 3, 2], strides=[-1, 4, 2], layout=Custom (0x0), const ndim=3

But if someone write like this:

let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13];let mut a = ArrayView::from_shape((2, 3, 2).strides(((-1isize as usize), 4, 2)), &s).unwrap();println!("{:?}", a);

This will cause unpredictable consequences:

[[[0, 2],  [4, 6],  [8, 10]], [[-622357902, 1],  [3, 5],  [7, 9]]], shape=[2, 3, 2], strides=[-1, 4, 2], layout=Custom (0x0), const ndim=3

The source of constructors with negative strides is the StrideShape trait, so I traversed the code and made corrections where StrideShape was used.

Fix constructors when strides are negative

2954979

Copy link

Member

bluss commentedJan 27, 2021•
edited
Loading

A test is missing here - but a test would also reveal that there is a debug assertion that ensures that these constructor calls are disallowed - so this change seems moot? (Ok, it seems this change is moot for the raw pointer constructors but not the slice constructor)

bluss reviewed

Jan 27, 2021

View reviewed changes

src/impl_raw_views.rs OutdatedShow resolvedHide resolved

implement constructors from negative strides

e49fc4a

Copy link

ContributorAuthor

SparrowLii commentedJan 28, 2021•
edited
Loading

That's right, from_shape_ptr should not use negative strides as pointer usage is too easy to be confused. I have made the correction. At present we have been able to master the use of negative strides to construct arrays, so I think it’s time to fix#842, which seems like clear: The only way for the user to construct an array by manually inputting negative strides is to construct StrideShape in from_shape/from/shape_vec. That is, <IntoDimension as ShapeBuilder>::strides(). (When strides is from Dimension, it is still usize, which is just fine. Because if we want to use another existing Dimension as a negative strides, then the value of this Dimension itself must already be negative). So I set the Strides type in IntoDimension of IntoStrides, which is the trait of using isize to generate strides to generate suitable Dim. The only difference between IntoDimension and its Strides is that the element is usize or isize. [usize;3] 's Strides is [isize;3]. Strides of Dimension is itself, it is just fine. This is compatible with the previous implementation.

SparrowLii changed the title~~Fix constructors when strides are negative~~Implement construction from negative strides

Jan 28, 2021

SparrowLii requested a review frombluss

February 3, 2021 13:22

Copy link

Member

bluss commentedFeb 7, 2021

The bug fix is great of course.

Allowing negative strides in constructors is a good next step but I haven't been able to comment on what a good design should be. It is a bit unfortunate that both signed and unsigned (dimension) inputs are allowed simultaneously, it could be that it's necessary?

Copy link

ContributorAuthor

SparrowLii commentedFeb 8, 2021•
edited
Loading

Allowing negative strides in constructors is a good next step but I haven't been able to comment on what a good design should be. It is a bit unfortunate that both signed and unsigned (dimension) inputs are allowed simultaneously, it could be that it's necessary?

@bluss I think it is reasonable to use unsigned (Dim[Ix; n]) in Dimension. But the parameters of dim and strides in the constructor should be unified as signed. In this way, we can add the same rules as in numpy: when a dim parameter is negative, the value of the parameter is jointly determined by other parameters.

import numpy as npa = np.arange(0, 12).reshape(3, 2, 2)b = np.arange(0, 12).reshape(3, 2, -2)c = np.arange(0, 12).reshape(-3, 2, 2)d = np.arange(0, 12).reshape(3, -2, 2)e = np.arange(0, 12).reshape(-3, 2, 2)f = np.arange(0, 12).reshape(3, 2, -1)g = np.arange(0, 12).reshape(3, 2, -3)h = np.arange(0, 12).reshape(3, 2, -9999)i = np.arange(0, 12).reshape(3, 2, -333)assert (a==b).all()assert (a==c).all()assert (a==d).all()assert (a==e).all()assert (a==f).all()assert (a==g).all()assert (a==h).all()assert (a==i).all()

In this case, we only need to modify the related implementation in IntoDimension, while also allowing negative strides

Copy link

Member

bluss commentedFeb 8, 2021•
edited
Loading

I think it is reasonable to use unsigned (Dim[Ix; n]) in Dimension.

Why? Specifically why unsigned strides are useful in this PR.

Signed dimensions is a different thing and I don't see a reason for allowing it here

Copy link

ContributorAuthor

SparrowLii commentedFeb 9, 2021•
edited
Loading

I'm sorry if I'm not thoughtful. I think that can minimize changes to the original implementation. I am not saying that signed dimension should be used. My idea is to maximize the convenience of users while keeping the original implementation of the dimension unchanged. And we only need to modify from_shape and shapebuilder.

The purpose of this pr is only to allow users to easily create negative strides. And I want to make a little improvement on this basis, so that signed values can be used when creating dim and strides.