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

Additional arithmetic operations/variants #697

Open
matzebond opened this issue Sep 3, 2019 · 10 comments
Open

Additional arithmetic operations/variants #697

matzebond opened this issue Sep 3, 2019 · 10 comments

Comments

@matzebond
Copy link

matzebond commented Sep 3, 2019

Hello
i get an type mismatch error when in do

let mut c_post: Array1<f32> = a.c.mapv(|x| x.tanh());
a.h = &a.o_gate * c_post;

Edit: o_gate has the type Array1<f32>

Error
error[E0271]: type mismatch resolving `<f32 as std::ops::Mul<ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>>>::Output == f32`
   --> src/main.rs:188:25
    |
188 |         a.h = &a.o_gate * c_post;
    |                         ^ expected struct `ndarray::ArrayBase`, found f32
    |
    = note: expected type `ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>`
               found type `f32`
    = note: required because of the requirements on the impl of `std::ops::Mul<ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>>` for `&ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>`

error[E0277]: the trait bound `ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>: ndarray::impl_ops::ScalarOperand` is not satisfied
   --> src/main.rs:188:25
    |
188 |         a.h = &a.o_gate * c_post;
    |                         ^ the trait `ndarray::impl_ops::ScalarOperand` is not implemented for `ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>`
    |
    = note: required because of the requirements on the impl of `std::ops::Mul<ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>>` for `&ndarray::ArrayBase<ndarray::OwnedRepr<f32>, ndarray::dimension::dim::Dim<[usize; 1]>>`

i think it tries to do a scalar operation

When i don't try to do it inplace it works

let mut c_post: Array1<f32> = a.c.mapv(|x| x.tanh());
a.h = &a.o_gate * &c_post;

We use inplace operations when concerned about the memory usage right?

@matzebond
Copy link
Author

To my surprise I also have to write
momentum.bf += &(&gradient.bf * &gradient.bf);
instead of momentum.bf += &gradient.bf * &gradient.bf;
more or less the same error as above (trying to do a scalar operation). All variables are Array1<f32>
Do I lack some understanding of references Rust?

@bluss
Copy link
Member

bluss commented Sep 4, 2019

hi, we can look at this if you provide example code for the problem or tell us the types of all the incoming variables

@matzebond
Copy link
Author

    let a: Array2<f32> = arr2(&[[2.0, 3.0], [4.0, 5.0]]);
    let c: Array2<f32> = arr2(&[[1.0, 0.0], [0.0, 1.0]]);
    let mut d: Array2<f32> = Array2::zeros((2,2));

    d = 2.0 * (&a / c.map(|x| x+1.0)); // &B @ B missing?
    d += b.t().dot(&a); // C @= B missing?
    d += &a * &a; // also C @= B 

    {
        let e = &mut d;
        *e = a.dot(&c); // works
        *e = &a.dot(&c); // why do i need to remove the borrow
        *e = e + a.dot(&a); // &mut B @ B missing?
        *e = a.dot(&a) + e; // B @ &mut B missing?
        *e = e + e; // &mut B @ &mut B missing?
    }

I am not sure if this me not understanding Rust or if there are some variants missing in impl_ops.rs.
In the comment I wrote which implementations might be missing in term os doc.
If you think that I am right I would give the implementation a shot.

@bluss
Copy link
Member

bluss commented Sep 4, 2019

Thanks! You're probably right - the ones you ask about as missing are not listed in the docs as being present:

https://docs.rs/ndarray/0.12.1/ndarray/struct.ArrayBase.html#arithmetic-operations

I'm not sure all of these should exist. I know it seems silly to say it like that — but adding one rule might result in dozens of additional impl blocks per rule.

Playground link for your example https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b5f6f01f0151d31d305a89e657810925

@matzebond
Copy link
Author

Are you concerned about the amount of code that will get generated?

@bluss
Copy link
Member

bluss commented Sep 4, 2019

Yes. Maybe there is a way to simplify it somehow.

@matzebond matzebond changed the title Inplace operation with map error Additional arithmetic operations/variants Sep 4, 2019
@LukeMathWalker
Copy link
Member

I do understand the bloat concern @bluss, but the error message you get from the compiler on this issue is particularly confusing compared to the simplicity of the operation you are trying to perform.

I bumped into it myself while I was working on another example for ndarray-examples and it took me a while to figure it out.

I'd be happy to work on this to expand the list of admitted operands combinations if you are willing to merge it/consider to merge it.

@bluss
Copy link
Member

bluss commented Sep 8, 2019

Let's make an issue to discuss this. There are loads of things that can be improved with the arithmetic ops.

@LukeMathWalker Let me know what you think. I've tried to make it specific #699

@bluss
Copy link
Member

bluss commented Sep 8, 2019

@LukeMathWalker I can't promise I have the bandwidth to approve it. We need to merge changes that I haven't approved too, otherwise we'll be stuck here :)

@LukeMathWalker
Copy link
Member

LukeMathWalker commented Sep 8, 2019

I guess what I meant was "unless you strongly feel against it" 👍 I'll contribute to the discussion in #699

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

No branches or pull requests

3 participants