Added stride support to Windows

[LazaroHurtado]

Currently the ArrayBase::windows() method creates an iterator of windows with a stride of 1 along all axes of the given array's dimensions. This is limiting the functionality of windows on arrays. One use case where having the ability to specify the stride is necessary is in convolutional neural networks, where what ndarray calls a window is basically a kernel.

Important notes

• Including a specified stride changes the size of the ArrayView found in the base field of the Windows struct. This change is derived from this formula.
• The ordering for the stride is (inner_most_axis, ..., outer_most_axis). This is because the axis that the windows gets pushed along first is the inner most axis... This was the original intention but noticing that the formula also follows outermost axis first, similar to indexing an ndarray, I believe it should be similar.

[nilgoyette]

Now that you created Windows::new_with_stride, shouldn't Windows::new simply calls Windows::new_with_stride with (1, ...)? If such a thing is possible, of course.

[LazaroHurtado]

Now that you created Windows::new_with_stride, shouldn't Windows::new simply calls Windows::new_with_stride with (1, ...)? If such a thing is possible, of course.

Great insight, changes have been applied, if there is a more effective way of creating a unit stride please let me know.

[LazaroHurtado]

@nilgoyette Please let me know if you would like me to change the windows doc description to refer to windows_with_stride with unit stride, similar to this.

[nilgoyette]

Yes, their description will be identical except for one detail. Good idea.

[LazaroHurtado]

Any more feedback would be highly appreciated! If collaborators would want me to squash all commits into one then would be happy to do so.

[nilgoyette]

Thank you for your contribution @LazaroHurtado

[bluss]

For some inputs this multiplication will wrap around. Check on input is needed so that the resulting array view is always valid in those and other cases, otherwise it's a "back door" into creating an invalid array view and UB.

[LazaroHurtado]

Good catch! I can release a follow-up PR using checked_mul and assert_ne! on the checked result with None ensuring the array view is valid, would you consider this sufficient?

[bluss]

I can write a longer comment, let's see what more there is. It is not exactly the solution.

[bluss]

@nilgoyette first of all I'm apologizing to you. I want to contribute net positive here (and not net negative) and chiming in like I did with stuff to second-guess anyone's review is not what I want to do. I want ndarray to live without me, and then it needs to merge stuff without my view of things have to be (which I repeat because that's what I've long accepted, but it's harder to do than say). 🙂

I see you're doing reviews and getting stuff merged and that's great 🙂. Be direct in what you need from me, because I might not be around to do it otherwise.

[bluss]

maybe a better name is base_strides

[bluss]

it's not a dimension but the stride value

[bluss]

We'll put in some comments here (this is my mental process now that I come back to this, I've implemented this, but maybe needed some comments.)

// base outlines the traversal: each element in base <=> each produced window's first element
// window and window strides determines size and strides of the array views produced

[bluss]

@LazaroHurtado I've rewritten my comment to be shorter and more clear (and hidden the old ones)

• array strides are signed quantities, stored in an usize field, and that's why we should avoid doing arithmetic on them explicitly. Here we'll have panics (debug mode) due to wraparound immediately if the user passes in any array with inverted axis (negative stride)

• Please use a higher level call to do the same operation, I found the slice axis inplace methods fit well for this, see below. Then the implementation details are taken into account correctly already.

We want to compute base by using a number of a.slice_axis_inplace(...) calls.
This is how to do it for the old code (before this PR, just needs expansion to the new features:)

let window_strides = a.strides.clone();

let mut base = a;
base.slice_each_axis_inplace(|ax_desc| {
    let len = ax_desc.len;
    let wsz = window[ax_desc.axis.index()];
    if len < wsz {
        Slice::new(0, Some(0), 1)
    } else {
        Slice::new(0, Some((len - wsz + 1) as isize), 1)
    }
});
Windows {
    base,
    window,
    strides: window_strides,
}

[LazaroHurtado]

Thank you @bluss for providing your feedback on this new feature, I highly appreciate it. I will take responsibility for making the proper changes to close out any potential bugs or improve code readability related to my work. Please be patient with me since I am still not 100% familiar with this crate and I have limited free time, nonetheless, I will deliver and keep you and @nilgoyette in the loop.

[nilgoyette]

Thank you @bluss for "refactoring" your comments. I don't know about@LazaroHurtado , but I was somewhat confused :|

As for the personnal comment

• I understand the apology, but it is not needed (!) You saw a bug that I didn't have the knowledge to see and you explained it. This is perfect. Honestly, I prefer having random educated comments from you than nothing.
• "Be direct in what you need from me" Well, my goal was to disturb you as less as possible (and even less than that) but you're the only person that knows the whole codebase and its dark corners so I guess I'll need to use the "@bluss" from time to time. When I do, I'll try to keep it short and clear.