Implement construction from negative strides #901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

SparrowLii wants to merge 2 commits into rust-ndarray:master

from SparrowLii:neg_strides

Open

Implement construction from negative strides #901

SparrowLii wants to merge 2 commits into rust-ndarray:master from SparrowLii:neg_strides

Conversation

@SparrowLii

Copy link

Contributor

@SparrowLii SparrowLii commented Jan 27, 2021

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.

@SparrowLii


 Fix constructors when strides are negative

@bluss

Copy link

Member

bluss commented Jan 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

bluss reviewed

Jan 27, 2021

View reviewed changes

src/impl_raw_views.rs Show resolved Hide resolved

@SparrowLii


 implement constructors from negative strides

e49fc4a

@SparrowLii

Copy link

Contributor Author

SparrowLii commented Jan 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 SparrowLii changed the title ~~(削除) Fix constructors when strides are negative (削除ここまで)~~ (追記) Implement construction from negative strides (追記ここまで)

Jan 28, 2021

@SparrowLii SparrowLii requested a review from bluss

February 3, 2021 13:22

@bluss

Copy link

Member

bluss commented Feb 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?

@SparrowLii

Copy link

Contributor Author

SparrowLii commented Feb 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 np
a = 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

@bluss

Copy link

Member

bluss commented Feb 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

@SparrowLii

Copy link

Contributor Author

SparrowLii commented Feb 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.