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.

Sign up for GitHub

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

Jump to bottom

NdProducer::Dim of axis_windows() should be Ix1 #1304

Open
jonasBoss opened this issue Jul 12, 2023 · 4 comments · May be fixed by #1460
Open

NdProducer::Dim of axis_windows() should be Ix1 #1304

jonasBoss opened this issue Jul 12, 2023 · 4 comments · May be fixed by #1460
Labels
enhancement

Comments

@jonasBoss
Copy link
Contributor

Please correct me if I am wrong, but it is my understanding that callingaxis_windows will return a Producer that traverses along the provided axis, not in any other axis. Therefore the associated type NdProducer::Dim should be Ix1, and the NdProducer::Item = ArrayView<'_, _, D>. Unfortunately they are both D.

I have a use-case where I need to zip some n-D data with a 1-D vector to calculate some result:

let data = array![
    [1,15],
    [2,14],
    [3,13],
    [4,12],
    [5,11],
    [6,10],
];
let x = array![10,20,30,40,50,60];
let mut result: Array<i32,_> = Array::zeros(data.raw_dim());

// everything except the first and last result we need to 
// calculate them seperately, because there is not sufficient data
let mut inner_result = result.slice_axis_mut(Axis(0), Slice::from(1..-1));

Zip::from(inner_result.axis_iter_mut(Axis(0)))
    .and(x.windows(3))
    .and(data.axis_windows(Axis(0), 3))
    .for_each(|y, x, data|{
        // calculate some result y_n with values from data_n-1, data_n and data_n+1
        // as well as x_n-1, x_n and x_n+1
        todo!()
    });
Error 
error[E0271]: type mismatch resolving `<Windows<'_, {integer}, Dim<[usize; 2]>> as IntoNdProducer>::Dim == Dim<[usize; 1]>`
   --> src/interp1d.rs:679:18
    |
679 |             .and(data.axis_windows(Axis(0), 3))
    |              --- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected an array with a fixed size of 1 element, found one with 2 elements
    |              |
    |              required by a bound introduced by this call
    |
    = note: expected struct `Dim<[usize; 1]>`
               found struct `Dim<[usize; 2]>`
note: the method call chain might not have had the expected associated types
   --> src/interp1d.rs:679:23
    |
662 |           let data = array![
    |  ____________________-
663 | |             [1,15],
664 | |             [2,14],
665 | |             [3,13],
...   |
668 | |             [6,10],
669 | |         ];
    | |_________- this expression has type `ArrayBase<OwnedRepr<{integer}>, Dim<[usize; 2]>>`
...
679 |               .and(data.axis_windows(Axis(0), 3))
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^ `IntoNdProducer::Dim` is `Dim<[usize; 2]>` here
note: required by a bound in `ndarray::Zip::<(P1, P2), D>::and`
   --> /home/jonas/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ndarray-0.15.6/src/zip/mod.rs:912:1
    |
912 | / map_impl! {
913 | |     [true P1],
914 | |     [true P1 P2],
915 | |     [true P1 P2 P3],
...   |
918 | |     [false P1 P2 P3 P4 P5 P6],
919 | | }
    | |_^ required by this bound in `Zip::<(P1, P2), D>::and`
    = note: this error originates in the macro `map_impl` (in Nightly builds, run with -Z macro-backtrace for more info)

I can not think of any reason why the dimension being D instead of Ix1 would be useful.
axis_iter() is essentially the same thing with the window size set to 1 and it has NdProducer::Dim = Ix1.

Actually the x in my example is not needed to produce the error, but it highlights the fact that I can not use axis_windows_mut(Axis(0), 1) (which does not exist for mut?) as a workaround.
@nilgoyette
Copy link
Collaborator

I'm not sure what you ask for.

  • inner_result.axis_iter_mut(Axis(0)) return [1x2] arrays
  • x.windows(3) returns 1x3 arrays
  • data.axis_windows(Axis(0), 3) returns 3x2 arrays
  • they all return 4 elements to iterate on (good)

The result of axis_windows looks like

[[1, 15],     [[2, 14],     [[3, 13],     [[4, 12],
 [2, 14],      [3, 13],      [4, 12],      [5, 11],
 [3, 13]]      [4, 12]]      [5, 11]]      [6, 10]]

But you clearly want a 1D array. What would it look like? If what you want is

[2, 14],     [3, 13],     [4, 12],     [5, 11],

then you're simply not using the right methods.

@bluss
Copy link
Member

bluss commented Jul 13, 2023

It's a reasonable request. The current status is because it's reusing the existing Windows producer.

The NdProducer Dim is the dimensionality of the space that the producer itself is travelling (not to be mixed with the dimensionality of elements produced). For axis windows, it is traversing a straight line, the axis, hence it can be 1d.

@jonasBoss jonasBoss mentioned this issue Jul 13, 2023
Merged
jonasBoss added a commit to jonasBoss/ndarray-interp that referenced this issue Jul 14, 2023
@jonasBoss
Workaround for axis_windows bug
893cbd6 
rust-ndarray/ndarray#1304
@goertzenator
Copy link

I'd like to add an adjacent feature request: axis_windows_with_stride().

My use case needs strided windows so I am trying to use windows_with_stride(), however that gives a producer with Dim=D (as it should) but that won't Zip with outer_iter() which has Dim=1... so same problem as above. An axis_windows_with_stride() method giving an NdProducer of Dim=1 would fix everything.

@goertzenator goertzenator linked a pull request Dec 4, 2024 that will close this issue
Open
@akern40
Copy link
Collaborator

akern40 commented Dec 13, 2024

@goertzenator seems like a reasonable request to me! Thanks for the PR; I'm going to mark that PR as closing this issue once it's merged.

@akern40 akern40 linked a pull request Dec 13, 2024 that will close this issue
add axis_windows_with_stride() method #1460
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add axis_windows_with_stride() method goertzenator/ndarray
5 participants
@goertzenator @nilgoyette @bluss @akern40 @jonasBoss