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.

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

Impl NdIndex<IxDyn> for &Vec<Ix> and Vec<Ix> #1146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bderrett
Copy link

@bderrett bderrett commented Jan 18, 2022

This change attempts to improve the ergonomics of indexing dynamic dimension arrays.

Let's say we have:

let mut a = ArrayD::<f32>::zeros(vec![1, 2]);
let index = vec![0, 0];

(Of course, in practice, the number of dims is dynamic.)

Then currently we can't do a[&index] = 1.0 or a[index] = 1.0. Instead we seem to need to do

let r: &[usize] = &index;
a[r] = 1.0;

This change makes the first two methods of indexing possible.

I'm new to ndarray, so there may be a discussion on this somewhere that I haven't seen. The new impls were created by copying the impl of &[Ix] (for &Vec) and by modifying the impl of &[Ix] (for Vec).

I can add tests for this if it looks useful?

@jturner314
Copy link
Member

Thanks for the PR. I agree that it may be worth adding Index implementations for Vec<usize> and &Vec<usize>; there isn't much downside. Alternatively, we could add implementations for Vec<usize> and &T where T: NdIndex<IxDyn>.

@bluss What do you think?

Fwiw, this currently works with ndarray:

let mut a = ArrayD::<f32>::zeros(vec![1, 2]);
let index = vec![0, 0];
a[&*index] = 1.0;

(Note the extra * to use the Vec's Deref implementation.)

If index is &Vec<usize>, it's a bit more awkward: a[&**index] = 1.0 or a[index.as_slice()] = 1.0.

Regarding the implementation -- I'd suggest converting to &[usize] and using the existing NdIndex<IxDyn> for &'a [Ix] implementation. (Look at NdIndex<IxDyn> for &'a IxDyn for a similar case.)

@bderrett
Copy link
Author

Many thanks for the comments. The PR now adds implementations for Vec<usize> and &T where T: NdIndex<IxDyn>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants