-
Notifications
You must be signed in to change notification settings - Fork 307
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
base: master
Are you sure you want to change the base?
Conversation
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) |
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. |
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? |
@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.
In this case, we only need to modify the related implementation in IntoDimension, while also allowing negative strides |
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 |
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. |
Can I cherry-pick the first commit from this PR into bugfixes in 0.15.? |
OK. glad it helps |
The bugfix part of this PR is needed for 0.15, but it's something I could handle with cherry-pick. 🙂 |
Is there something I need to do ? |
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:
Bacuase of #885, this is just all right:
But if someone write like this:
This will cause unpredictable consequences:
The source of constructors with negative strides is the StrideShape trait, so I traversed the code and made corrections where StrideShape was used.