Skip to content
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

Implement construction from negative strides #901

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SparrowLii
Copy link
Contributor

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.

@bluss
Copy link
Member

bluss commented Jan 27, 2021

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)

src/impl_raw_views.rs Outdated Show resolved Hide resolved
@SparrowLii
Copy link
Contributor Author

SparrowLii commented Jan 28, 2021

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

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

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

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.

@bluss
Copy link
Member

bluss commented Mar 13, 2021

Can I cherry-pick the first commit from this PR into bugfixes in 0.15.?

@SparrowLii
Copy link
Contributor Author

Can I cherry-pick the first commit from this PR into bugfixes in 0.15.?

OK. glad it helps

@bluss bluss added this to the 0.15.0 milestone Mar 14, 2021
@bluss
Copy link
Member

bluss commented Mar 14, 2021

The bugfix part of this PR is needed for 0.15, but it's something I could handle with cherry-pick. 🙂

@SparrowLii
Copy link
Contributor Author

Is there something I need to do ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants