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

CowArray feature #632

Merged
merged 12 commits into from May 30, 2019
Merged

CowArray feature #632

merged 12 commits into from May 30, 2019

Conversation

andrei-papou
Copy link
Contributor

No description provided.

Andrew and others added 2 commits April 3, 2019 11:54
tests/array.rs Outdated
S2: Data<Elem=A>,
D: Dimension
{
arr1.iter().zip(arr2.iter()).all(|(x1, x2)| x1 == x2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you can't use the good old assert_eq!? Your first usage looks like

assert!(is_content_identical(&arr_cow, &expected_arr));

and I don't understand why assert_eq! can't do the job.

tests/array.rs Outdated
}

#[test]
fn test_is_owned() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only a suggestion, but I would merge test_is_view and test_is_owned because one is simply the reverse of the other,

@andrei-papou
Copy link
Contributor Author

@nilgoyette thanks for your notes, I've updated the PR.

@andrei-papou
Copy link
Contributor Author

@jturner314 any feedback on this? Once it's merged, I will be able to update #616 .

@jturner314
Copy link
Member

I'm sorry for the delay. Life has been busy recently. I've created andrei-papou#7 with the following:

  • Apply the tests to arrays with non-standard layouts.
  • Add/improve documentation.
  • Minor implementation changes.
  • Rename ArrayCow to CowArray. The name CowArray seems a little more natural to me, and it seems to fit better with ArcArray/RawArrayView/RawArrayViewMut. It's also nice that alphabetic sorting moves CowArray after e.g. ArrayView.

Once that's merged, I think we're good-to-go on this PR.

@jturner314 jturner314 changed the title ArrayCow feature CowArray feature May 30, 2019
@jturner314 jturner314 merged commit 5b5d898 into rust-ndarray:master May 30, 2019
@jturner314
Copy link
Member

Yay! I've been wanting this feature for quite a while. Thanks for working on it!

@SuperFluffy
Copy link
Contributor

Excellent, this brings me one step closer to being able to pass in slices containing arrays, like &[&ArrayBase<S, D>], where S: Data, D: Dim.

I believe that right now, due to monomorphisation, I wouldn't actually be able to pass in a mixture of Array and ArrayView, e.g. &[&Array2<f64>, &ArrayView2<f64>, &Array2<f64>].

I have to play around with it, but I don't think this feature will completely fix this either, because the trait bound fn f<T: Into<CowArray>>(myarrays: &[&T]) won't work for mixed T, again due to monomorphization.

Might a Deref<Target=ArrayView> for Array help?

@andrei-papou
Copy link
Contributor Author

andrei-papou commented May 31, 2019

@SuperFluffy I'm not sure &[&Array2<f64>, &ArrayView2<f64>, &Array2<f64>] will work with Deref<Target=ArrayView> for Array because you still need all the members of a slice to be of the same type. &Array2<f64> and &ArrayView2<f64> are completely different types from the compiler's point of view.
IMHO what you need in your case is dynamic dispatch, but it is not available for ArrayBase. You cannot write ArrayBase<dyn Data, dyn Dim> just because S and D type arguments of ArrayBase are Sized by default.

@jturner314
Copy link
Member

Right, all the elements in a slice need to have the same type. @SuperFluffy It seems like you want to be able to do something like this, but for ArrayView and Array instead of &[A] and Vec<A>?

let slice = &[1, 2, 3][..];
let vec = vec![4, 5, 6];
let collection = &[slice, &vec];

Note that this doesn't work with

let collection = &[&slice, &vec];

which is the statement analogous to let collection = &[&view, &owned];.

An implementation of Deref<Target = ArrayView> for ArrayBase would be pretty dangerous because we'd effectively have to bitwise re-interpret a portion of the ArrayBase as an ArrayView. (I think it would be possible to do correctly if we make ArrayBase #[repr(C)], but it's pretty tricky.)

For arrays, why not just call .view() on all the arrays and create a slice of ArrayViews (all the same type)? Like this:

let collection = &[arr1.view(), arr2.view()];

If you need mutable views, then use .view_mut() instead.

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

Successfully merging this pull request may close these issues.

None yet

4 participants