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

Implement Add<&Self> for all types #145

Closed
gsingh93 opened this issue Jul 16, 2015 · 7 comments
Closed

Implement Add<&Self> for all types #145

gsingh93 opened this issue Jul 16, 2015 · 7 comments

Comments

@gsingh93
Copy link

This would allow adding a Vec3 with a &Vec3. This will help reduce unnecessary copies, which in some cases can lead to non-trivial performance gains.

@sebcrozet
Copy link
Member

Do you have an example where such performance gains can be observed ? I tend to think that llvm would manage to inline the addition whenever the cost of copying (and function call) could be an issue.

@gsingh93
Copy link
Author

You're right, in the few cases I tried, LLVM did a good job of optimizing the copying away.

I still think this might be good to add for a few reasons.

  1. Performance of debug builds will improve. Obviously this isn't as important as release builds, but it can help save on development time.
  2. My test cases weren't that complicated. I can imagine some scenario where LLVM isn't able to inline the function call.
  3. It makes sense logically and ergonomically. I may pass in a reference to a function (maybe it's a &mut reference so it can be modified) that's involved in some addition. I'd rather just use the reference as is instead of dereferencing it on every addition.

I don't see any downsides of adding this, and while the upsides are minor, they're not trivial.

@yongqli
Copy link

yongqli commented Jul 23, 2015

👍

This would be useful for large DVecs, as you don't really want to copy them.

@sebcrozet
Copy link
Member

It looks like the standard library does implement Add, Mul, etc. for the basic types as well as references to them. So it now seems coherent for us to do the same for our own algebraic types.

@sebcrozet
Copy link
Member

Fixed by #218.

@ghost
Copy link

ghost commented Apr 26, 2017

Did I miss some impl in the docs?

log:

binary operation `+` cannot be applied to type `&'a mut nalgebra::Matrix<f64, nalgebra::Dynamic, nalgebra::U1, nalgebra::MatrixVec<f64, nalgebra::Dynamic, nalgebra::U1>>` [E0369]
an implementation of `std::ops::Add` might be missing for `&'a mut nalgebra::Matrix<f64, nalgebra::Dynamic, nalgebra::U1, nalgebra::MatrixVec<f64, nalgebra::Dynamic, nalgebra::U1>>` [E0369]

code excerpt:

// type declaration from function signature: filter_coefs: &'a mut DVector<f64>, signal: &'a [f64]
//
let sig_slice: &'a _ = &signal[sig_ind..slice_end];
let sig_sub = DVector::from_column_slice(1, sig_slice);
let cur_err = signal[sig_ind] - filter_coefs.dot(&sig_sub);
filter_coefs = filter_coefs + step_size * cur_err * sig_sub.clone() / sig_sub.dot(&sig_sub);

nalgebra = "0.12.0"

Fixable by iterating through the inds, but that seems sad:

let delta = step_size * cur_err * sig_sub.clone() / sig_sub.dot(&sig_sub);
for ind in 0..filter_coefs.len() {
    filter_coefs[ind] += delta[ind]
}

@sebcrozet
Copy link
Member

This must be because you are using a mutable reference. The following should work (note the &*filter_coefs):

*filter_coefs = &*filter_coefs + step_size * cur_err * sig_sub.clone() / sig_sub.dot(&sig_sub);

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