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.

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.

@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.

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

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.

Thank you for your contribution @LazaroHurtado

@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.

@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,
}

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.

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.