Change `NdProducer::Dim` of `axis_windows()` to `Ix1` #1305

jonasBoss · 2023-07-13T09:15:37Z

This breaks the public API because the method axis_windows() now returns a new AxisWindows struct. And the dimension of the NdProducer::Dim will always be Ix1.

If the old behavior is needed the method windows() can be used.

I was thinking about adding a method AxisWindows::into_windows(self) -> Windows but I don't think that this will see much use. Let me know if i should implement it.

adamreichold · 2023-07-13T09:53:48Z

src/iterators/windows.rs

+}
+
+
+impl<'a, A, D: Dimension> NdProducer for AxisWindows<'a, A, D> {


Is the impl_ndproducer! macro unable to handle this impl? (It seems to be used for Windows.)

almost, the methods raw_dim, uget_ptr, stride_of and split_at need a different implementation.

adamreichold · 2023-07-13T09:55:08Z

src/iterators/windows.rs

+        let mut window = base.raw_dim();
+        window[axis_idx] = window_size;
+
+        base.slice_each_axis_inplace(|ax_desc| {


I am not really happy about the code duplication between this method and Windows::new_with_stride. Is it reasonably possible to have re-use without making this too hard to follow? (Windows::new_with_stride also appears to have additional assertions/consistency checks?)

There is a way to do this with reusing the Windows struct. I don't know if it is more or less clean. It would look like this:

pub struct Windows<'a, A, D, V>{ base: ArrayView<'a, A, D>, window: D, strides: D, version: V, } pub struct GeneralWindow; pub struct AxisWindow{index: usize} impl<'a, A, D: Dimension> NdProducer for Windows<'a, A, D, GeneralWindow> { // the default impl } impl<'a, A, D: Dimension> NdProducer for Windows<'a, A, D, AxisWindow> { // the impl for the Axis version }

Now the methods axis_windows and windows would both return Windows but with a different generic V.

The struct GeneralWindow and AxisWindow need to be public, but they can be inaccessible from outside of the crate.

The assertions are not needed for AxisWindows::new because we construct the variables strides and window locally. axsis_windows() asserts that the axis is in range.

The struct GeneralWindow and AxisWindow need to be public, but they can be inaccessible from outside of the crate.

This does sound reasonable to me

I would suggest making Windows and AxisWindows public and have both wrap an WindowsImpl or something like that. (Meaning AxisWindows contains two fields, index: usize and impl: WindowsImpl.)

I would also suggest replacing the term "version" by "variant" to avoid any confusion as "version" often has a temporal connotation.

I don't know if it is more or less clean.

Of course, if this just increases complexity for very little gain in the end, it might not be worth it, you just have to give it a try.

As an alternative, a simple helper function encapsulating the slice-in-place approach using references to the constituent fields of Windows and AxisWindows might work as well.

I just updated to my suggested structure, It actually does reduce code duplication a lot.

I am not a fan of exposing these implementation detail generics in the API, hence the suggestion to expose Windows and AxisWindows with the same generic parameters as before which contain a WindowsImpl/Inner/Whatever as a private field. We should not leak this sharing of implementation in the public API.

I thought that This does sound reasonable to [you] 🙁

We need to expose that there is a difference between the return type of windows() and axis_windows() there is no way around that.

This generic is not an implementation detail. It makes a real difference to the public API. Specifically it changes the behavior of <Windows as NdProducer> and the Type of <Windows as NdProducer>::Dim. Otherwise this PR would be pointless.

Now we have this solution which is as minimal as it gets.
Or the solution I had before. Sure I could try and faff around with some helper methods and structs to reduce code duplication for the cost of clarity and more boilerplate.
Having a WindowsInner/Impl/... will break the impl_ndproducer! macro, because base needs to be a field of Windows not of some wrapped type.

We need to expose that there is a difference between the return type of windows() and axis_windows() there is no way around that.

Distinct return types is not the issue. It is return types which only differ in a type parameter which I have a problem with. I want the relation between these two return types to be fully opaque to the caller, so that e.g. we could also not have code reuse in their implementation without that affecting our API.

This generic is not an implementation detail. It makes a real difference to the public API. Specifically it changes the behavior of and the Type of ::Dim. Otherwise this PR would be pointless.

<Windows as NdProducer>::Dim and <AxisWindows as NdProducer>::Dim can be different without the outside world seeing any relation between those two types.

Having a WindowsInner/Impl/... will break the impl_ndproducer! macro, because base needs to be a field of Windows not of some wrapped type.

This is a real issue and maybe manually implementing NdProducer as you did for AxisWindows for both cases is simple enough, but maybe this actually reduces reuse. This is why I also suggested just using an external helper method with completely disjoint struct types.

Thank you for clearing that up. This makes much more sense.

I just extracted the main logic of creating the base into a separate function, and call that from both constructors Windwos::new_with_stride() and AxisWindows::new()

jonasBoss · 2023-07-13T12:12:05Z

src/iterators/windows.rs

+    fn stride_of(&self, axis: Axis) -> isize {
+        assert_eq!(axis, Axis(0));
+        self.base.stride_of(Axis(self.axis_idx))
+    }
+
+    fn split_at(self, axis: Axis, index: usize) -> (Self, Self) {
+        assert_eq!(axis, Axis(0));
+        let (a, b) = self.base.split_at(Axis(self.axis_idx), index);


Here I added assertions to check that this is only called with Axis(0). Do we need this? I could just ignore the axis argument, but that would be confusing when a user tries to call this with a different axis.

In general I am not super confident that this trait is correctly implemented, as there is a severe lack of documentation for a lot of the methods.

Specialize the `Windows` struct with a `variant` field to replace the `AxisWindows` struct

This reverts commit d5d7c72.

jonasBoss · 2023-08-18T16:14:03Z

Is there anything holding this back that I can do do address?

The CI errors don't seem to be related to this PR.

adamreichold · 2023-08-18T16:55:19Z

Is there anything holding this back that I can do do address?

The CI errors don't seem to be related to this PR.

No. Sorry this is waiting on me finding time to look into it again.

jonasBoss added 3 commits July 13, 2023 00:08

create AxisWindows struct and implement it

a8d7f79

fix some bugs, make the tests pass

6b0942f

add a test for zipping axis_windows with a 1d array

d3460e8

adamreichold reviewed Jul 13, 2023

View reviewed changes

jonasBoss commented Jul 13, 2023

View reviewed changes

bluss added the breaking-change label Jul 13, 2023

Reduce code duplication

d5d7c72

Specialize the `Windows` struct with a `variant` field to replace the `AxisWindows` struct

jonasBoss requested a review from adamreichold July 14, 2023 07:46

jonasBoss added 2 commits July 14, 2023 15:13

Revert "Reduce code duplication"

51bccf9

This reverts commit d5d7c72.

move core logic of creating the Windows logic into seperate fn

859cc2b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `NdProducer::Dim` of `axis_windows()` to `Ix1` #1305

Change `NdProducer::Dim` of `axis_windows()` to `Ix1` #1305

jonasBoss commented Jul 13, 2023

adamreichold Jul 13, 2023

jonasBoss Jul 13, 2023

adamreichold Jul 13, 2023

jonasBoss Jul 13, 2023

adamreichold Jul 13, 2023

jonasBoss Jul 14, 2023

adamreichold Jul 14, 2023

jonasBoss Jul 14, 2023

adamreichold Jul 14, 2023 •

edited

jonasBoss Jul 14, 2023

jonasBoss Jul 13, 2023

jonasBoss commented Aug 18, 2023

adamreichold commented Aug 18, 2023

		}


		impl<'a, A, D: Dimension> NdProducer for AxisWindows<'a, A, D> {

Change NdProducer::Dim of axis_windows() to Ix1 #1305

Are you sure you want to change the base?

Change NdProducer::Dim of axis_windows() to Ix1 #1305

Conversation

jonasBoss commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreichold Jul 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasBoss commented Aug 18, 2023

adamreichold commented Aug 18, 2023

Change `NdProducer::Dim` of `axis_windows()` to `Ix1` #1305

Change `NdProducer::Dim` of `axis_windows()` to `Ix1` #1305

adamreichold Jul 14, 2023 •

edited