-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add MulAdd and MulAddAssign #44
Conversation
On my machine test are passing with
Given that the CI test fails only confirm my initial thought that I'm doing something wrong. |
I would avoid the macros in this case, because Given 3 operands, there are 8 possible combinations of value/reference operands here. I'm not sure it's worth trying to support them all. Using all by-value operands is definitely important, and all by-reference may be useful for non-Copy types. The rest of the combinations are debatable and probably not worth the extra complication here. |
6b06daf
to
a13e374
Compare
The test fails on I know there is
But would this not interfere with with the test when we test with floats?
|
We know |
Thanks a lot Josh, for the help. I'm pretty sure that there still are somethings I can do better but at-least the test are green. Let me know what you think. |
src/lib.rs
Outdated
@@ -539,6 +539,27 @@ impl<T: Clone + Num> Mul<Complex<T>> for Complex<T> { | |||
} | |||
} | |||
|
|||
|
|||
// (a + i b) - (c + i d) == (a - c) + i (b - d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write the correct expansion here, for (a + i b) * (c + i d) + (e + i f)
. You can show multiple steps, e.g. first fully expanded, then factored such that it's clear where inner mul_add
can be applied.
src/lib.rs
Outdated
@@ -601,6 +622,12 @@ mod opassign { | |||
} | |||
} | |||
|
|||
impl<T: Clone + NumAssign + MulAdd<Output = T>> MulAddAssign for Complex<T> { | |||
fn mul_add_assign(&mut self, other: Complex<T>, add: Complex<T>) { | |||
*self = self.clone().mul_add(other, add); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this do any better using T: MulAddAssign
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but I just get this error.
error[E0599]: no method named `mul_add` found for type `Complex<T>` in the current scope
--> src/lib.rs:708:34
|
83 | pub struct Complex<T> {
| --------------------- method `mul_add` not found for this
...
708 | *self = self.clone().mul_add(other.clone(), add.clone());
| ^^^^^^^
|
= note: the method `mul_add` exists but the following trait bounds were not satisfied:
`&Complex<T> : traits::MulAdd<_, _>`
= help: items from traits can only be used if the trait is implemented and in scope
= note: the following traits define an item `mul_add`, perhaps you need to implement one of them:
candidate #1: `traits::Float`
candidate #2: `traits::MulAdd`
candidate #3: `traits::real::Real`
The only way I could solve it was with MulAdd<Output = T>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd also have to adjust the implementation, something like self.re.mul_add_assign(...)
etc. - taking care to separate other operations that will need that original value.
It may not be any better, just something to try. We could also just leave ourselves open to this possibility in the function signature, by requiring both MulAddAssign
and MulAdd
even if we only use the latter for now.
3b2ff07
to
ed65a51
Compare
I haven't impl all the forward macros on MulAdd and MulAddAssign. |
8994297
to
2949f15
Compare
Sorry I let this go so long. Hope you don't mind that I took the liberty to rebase and squash your commits, and do a little followup myself. Thanks for bearing with me! bors r+ |
44: Add MulAdd and MulAddAssign r=cuviper a=Schultzer Hi Josh, I'm not really sure with this impl I feel like I do a lot of things wrong Right now I get it to compile and work on some tests. But macros are not my forte. I hope you can point me in the right direction. Closes #37 Co-authored-by: Benjamin Schultzer <benjamin@schultzer.com> Co-authored-by: Josh Stone <cuviper@gmail.com>
Build succeeded |
@cuviper No not at all I'm here to learn, Thanks. |
Hi Josh,
I'm not really sure with this impl I feel like I do a lot of things wrong
Right now I get it to compile and work on some tests.
But macros are not my forte.
I hope you can point me in the right direction.
Closes #37