Skip to content

Non-assignment binary operations shouldn't mutate input #83

Closed
Closed
Non-assignment binary operations shouldn't mutate input#83

Description

@SuperFluffy
SuperFluffy
opened on Feb 13, 2016

In the example at the bottom, addition of two arrays yields a new one, but also mutates one of the inputs. This is because the add() function in the Add trait is defined in terms of the in-place iadd() (from the macro impl_binary_op macro):

impl<'a, A, S, S2, D, E> $trt<&'a ArrayBase<S2, E>> for ArrayBase<S, D>
    where A: Clone + $trt<A, Output=A>,
          S: DataMut<Elem=A>,
          S2: Data<Elem=A>,
          D: Dimension,
          E: Dimension,
{
    type Output = ArrayBase<S, D>;
    fn $mth (mut self, rhs: &ArrayBase<S2, E>) -> ArrayBase<S, D>
    {
        self.$imth(rhs);
        self
    }
}

This is fine, if self is an OwnedArray or an Array because they are consumed anyways, but if instead an ArrayViewMut is passed in, the underlying data changes. Is this intended? I think one shouldn't expect the array to get changed when addition is performed on a view.

extern crate ndarray;

use ndarray::{Array,OwnedArray};

fn main() {
    let r = 0..4;
    let v = r.map(|x| x as f64).collect::<Vec<_>>();

    let mut a = OwnedArray::from_vec(v.clone()).into_shape((2,2)).unwrap();
    let b = OwnedArray::from_vec(v.clone()).into_shape((2,2)).unwrap();

    let bv = b.view();

    {
        let x = a.view_mut() + bv;
        println!("{:?}", x);
    }

    println!("{:?}", a);
}

Activity

  1. bluss commented on Feb 14, 2016

    @bluss
    bluss
    on Feb 14, 2016
    Member

    I guess it's intentional, I didn't think too much about this choice.

    Which do you expect:

    • Unsupported operation?
    • Returns an OwnedArray?

    As it is, it's useful to some extent since it allows a + b + c + d where a is a mutable view.

    That would have to be replaced by a += &b; a += c; a += &d; or a.iadd(&b); a.iadd(&c); a.iadd(&d);. The former with += is ok, the iadds are a bit noisy..

  2. bluss commented on Feb 14, 2016

    @bluss
    bluss
    on Feb 14, 2016
    Member

    Unfortunately we will maybe not get rid of the iadd even when += is stable, because += is not applicable on rvalues, i.e. in this situation:

    a.slice_mut(s![..4, ..4]) += 1.0; // ERROR

  3. SuperFluffy commented on Feb 14, 2016

    @SuperFluffy
    SuperFluffy
    on Feb 14, 2016
    ContributorAuthor

    Isn't there the 3rd option of never doing the operation in place and instead cloning the array, doing the operations on the clone, and then returning it?

    I don't necessarily expect A @ B to be in-place anyways and so performance is secondary.

    For rvalues, there are already some examples in the tests, which demand binding the view to an lvalue first. While needing one more line, I think doing the below is fine:

    let mut asliced = a.slice_mut(s![..4, ..4]);
asliced += 1.0;

    But I have a feeling I misunderstood the issue.

  4. bluss commented on Feb 14, 2016

    @bluss
    bluss
    on Feb 14, 2016
    Member

    That sounds like the second alternative, return OwnedArray.

  5. bluss commented on Feb 14, 2016

    @bluss
    bluss
    on Feb 14, 2016
    Member

    Which way would have performance as primary objective? I think a + b must be good. Right now &a + &b creates a new array for the result and a + b reuses the allocation in a.

  6. bluss commented on Feb 26, 2016

    @bluss
    bluss
    on Feb 26, 2016
    Member

    @vbarrielle What do you think?

  7. vbarrielle commented on Feb 26, 2016

    @vbarrielle
    vbarrielle
    on Feb 26, 2016
    Contributor

    I haven't played with the binary operations for the moment, I'll have a look.

  8. vbarrielle commented on Feb 28, 2016

    @vbarrielle
    vbarrielle
    on Feb 28, 2016
    Contributor

    I've been playing a bit with @SuperFluffy's example. I find the behaviour on mutable views surprising,it might be a pain point for users.
    On the other hand I really like the current behaviour of requiring let res = a.clone() + b.view() to get an OwnedArray, it makes the allocation explicit.

    I also feel that having let x = a.view_mut() + bv; mutate a can be usefull in some cases, ie there are situations where you really don't wan't to allocate.

    I'm thinking there could be a way to keep this feature without having a surprising behaviour:

    • don't implement binary operations on ArrayViewMut, preventing that case from compiling
    • create a newtype over ArrayViewMut, on which the binary operations are implemented.
    • let arrays create such an "in place operations view", enabling patterns such as let _ = a.iop_mut() + b.view() + c.view() + d.view()

  9. bluss commented on Feb 28, 2016

    @bluss
    bluss
    on Feb 28, 2016
    Member

    There's a summary here actually https://bluss.github.io/rust-ndarray/master/ndarray/struct.ArrayBase.html#arithmetic-operations

    I'm reading your thoughts, but I wanted to say that there are more combinations, for example that &A + &A always creates a new OwnedArray without explicit clone/to_owned.

  10. bluss commented on Feb 28, 2016

    @bluss
    bluss
    on Feb 28, 2016
    Member

    We can easily take the step to remove the ArrayViewMut impls, just to test what happens.

    Is an in place operations view necessary? The user still as .iadd() and += available.

  11. vbarrielle commented on Feb 28, 2016

    @vbarrielle
    vbarrielle
    on Feb 28, 2016
    Contributor

    No it's not necessary, repeated calls to iadd are fine imo. += is not yet in stable rust is it?

    Simply removing the impl on ArrayViewMut would be a good move imho.

    I'm reading your thoughts, but I wanted to say that there are more combinations, for example that &A + &A and &A + B always create a new OwnedArray without explicit clone/to_owned.

    Ok I had not seen those. Well I think it still makes the allocation somehow explicit, ie there is no way the result could be written into A.

    In summary: I like the current implementation of binary operations, except for the ArrayViewMut impl part.

  12. bluss commented on Feb 28, 2016

    @bluss
    bluss
    on Feb 28, 2016
    Member

    Thank you for your thoughts! I think we will remove the ViewMut impls. I'll update the arithmetic ops overview, I think it's quite necessary to have a summary when there are so many impl clauses.

    There is very good news -- all in place operators are being stabilized for Rust 1.8.

  13. vbarrielle commented on Feb 28, 2016

    @vbarrielle
    vbarrielle
    on Feb 28, 2016
    Contributor

    It's nice that in place operators are getting stabilized! Can't wait to use them.

  14. SuperFluffy commented on Feb 28, 2016

    @SuperFluffy
    SuperFluffy
    on Feb 28, 2016
    ContributorAuthor

    Maybe with impl specialization landing sometime in the future, the impls on ViewMut could then have a different behaviour?

  15. bluss
    added a commit that references this issue on Feb 28, 2016

    Arithmetic operations B @ A are now only implemented for owned storag…

    1dc4bfd

  16. bluss commented on Feb 28, 2016

    @bluss
    bluss
    on Feb 28, 2016
    Member

    Not sure if specialization impacts this or not. Maybe it can be implemented with a different behavior already now.

    However, the use of generics in this crate is already very complicated. We want to have some logical system that the users will understand — it seems easier to just leave this unimplemented in that case.

  17. bluss
    closed this as completedin #93on Feb 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions