Implement Neg and Not for &ArrayBase

[jturner314]

No description provided.

[bluss]

These are not really comments on your PR, but it's imperfections in existing operator impls that are catching up with us. It explains where we are and what we want to improve.

• This is an example of where we will use Rust 1.21's needs_drop so that we can elide copying in .to_owned() (making an uninit array) and make all these operations on array references more efficient.

  • What's more efficient than copying a whole array and then negating it? Making a new array and writing in negated values as we go (assuming both those iterations can be made with the same efficiency).

• We need to handle non-primitives better. Imagine BigNums; ideally we want to negate them here by passing in a reference to a bignum and getting a new owned bignum value back. (Instead of cloning it and then negating it).

[jturner314]

This is an example of where we will use Rust 1.21's needs_drop so that we can elide copying in .to_owned() (making an uninit array) and make all these operations on array references more efficient.

How does needs_drop help here? I don't see what you're getting at.

We need to handle non-primitives better. Imagine BigNums; ideally we want to negate them here by passing in a reference to a bignum and getting a new owned bignum value back. (Instead of cloning it and then negating it).

We could do

impl<'a, A, S, D> Neg for &'a ArrayBase<S, D>
where &'a A: Neg<Output=A>, ...

and then use ArrayBase.map() to avoid making any clones. At a minimum, this would work for the number types in the standard library and the num-bigint crate. What do you think?

[bluss]

How does needs_drop help here? I don't see what you're getting at.

It the non-needs drop case makes it much easier to use uninitialized data (Array::uninitialized()) without losing an arm and a leg, so it's mainly if we can exploit that. .map() sounds good here!

[jturner314]

I changed the implementation to use .map() so that it no longer clones all the data.

You may notice that I also removed the generic parameter A on test_oper_arr(). I tried keeping it and adding a &'a A: 'a + Neg<Output=A> constraint (and a number of other variations) but I kept running into this error:

error[E0275]: overflow evaluating the requirement `<_ as ndarray::Data>::Elem`
  --> tests/oper.rs:22:5
   |
22 |     test_oper_arr(op, aa.clone(), bb.clone(), cc.clone());
   |     ^^^^^^^^^^^^^
   |
   = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required because of the requirements on the impl of `std::ops::Neg` for `&ndarray::ArrayBase<_, _>`
   = note: required by `test_oper_arr`

The help for the error indicates that there's a recursive trait bound, but I don't see it. If you can figure out what's going on, please let me know. Otherwise, changing A to f32 was a successful workaround.

[jturner314]

It turns out that it's possible to avoid the error by manually specifying the type of A when calling test_oper_arr(). Many thanks to trentj on IRC for pointing this out! I just pushed a commit that adds the A type parameter again.

[bluss]

Looks neat, map() is doing all the work for us. Thanks!