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

Add MulAdd and MulAddAssign #44

Merged
merged 3 commits into from
May 1, 2019

Conversation

Schultzer
Copy link
Contributor

@Schultzer Schultzer commented Feb 26, 2019

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

@Schultzer
Copy link
Contributor Author

On my machine test are passing with

warning: function cannot return without recursing
   --> src/lib.rs:510:13
    |
510 |             fn mul_add(self, other: &Complex<T>, add: Complex<T>) -> Complex<T> {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
511 |                 self.mul_add(other, add)
    |                 ------------------------ recursive call site
...
576 | forward_all_binop!(impl MulAdd, mul_add);
    | ----------------------------------------- in this macro invocation
    |
    = note: #[warn(unconditional_recursion)] on by default
    = help: a `loop` may express intention better if this is on purpose

Given that the CI test fails only confirm my initial thought that I'm doing something wrong.

@cuviper
Copy link
Member

cuviper commented Feb 26, 2019

I would avoid the macros in this case, because MulAdd doesn't fit the same repetition as the binary operators. Just write out the variations on their own, then let's see what you get.

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.

@Schultzer
Copy link
Contributor Author

Schultzer commented Feb 26, 2019

The test fails on cargo test --no-default-features doesn't this mean that MulAdd and MulAddAssign is only impl for std for f64 and f32?

I know there is

pub trait MulAdd<A = Self, B = Self> {
    /// The resulting type after applying the fused multiply-add.
    type Output;

    /// Performs the fused multiply-add operation.
    fn mul_add(self, a: A, b: B) -> Self::Output;
}

/// The fused multiply-add assignment operation.
pub trait MulAddAssign<A = Self, B = Self> {
    /// Performs the fused multiply-add operation.
    fn mul_add_assign(&mut self, a: A, b: B);
}

But would this not interfere with with the test when we test with floats?

#[cfg(feature = "std")]
impl MulAdd<f32, f32> for f32 {
    type Output = Self;

    #[inline]
    fn mul_add(self, a: Self, b: Self) -> Self::Output {
        f32::mul_add(self, a, b)
    }
}

#[cfg(feature = "std")]
impl MulAdd<f64, f64> for f64 {
    type Output = Self;

    #[inline]
    fn mul_add(self, a: Self, b: Self) -> Self::Output {
        f64::mul_add(self, a, b)
    }
}

#[cfg(feature = "std")]
impl MulAddAssign<f32, f32> for f32 {
    #[inline]
    fn mul_add_assign(&mut self, a: Self, b: Self) {
        *self = f32::mul_add(*self, a, b)
    }
}

#[cfg(feature = "std")]
impl MulAddAssign<f64, f64> for f64 {
    #[inline]
    fn mul_add_assign(&mut self, a: Self, b: Self) {
        *self = f64::mul_add(*self, a, b)
    }
}

@cuviper
Copy link
Member

cuviper commented Feb 27, 2019

We know MulAdd only works for floats with std, so just cfg that test function. You could add separate integral tests for the no_std case.

@Schultzer Schultzer changed the title [WIP] Add MulAdd and MulAddAssign Add MulAdd and MulAddAssign Feb 27, 2019
@Schultzer
Copy link
Contributor Author

Schultzer commented Feb 27, 2019

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 Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
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)
Copy link
Member

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);
Copy link
Member

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?

Copy link
Contributor Author

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>.

Copy link
Member

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.

@Schultzer Schultzer force-pushed the add-mul-add-mul-add-assign branch 2 times, most recently from 3b2ff07 to ed65a51 Compare March 30, 2019 03:56
@Schultzer
Copy link
Contributor Author

Schultzer commented Mar 30, 2019

I haven't impl all the forward macros on MulAdd and MulAddAssign.

@cuviper cuviper force-pushed the add-mul-add-mul-add-assign branch from 8994297 to 2949f15 Compare May 1, 2019 23:17
@cuviper
Copy link
Member

cuviper commented May 1, 2019

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+

bors bot added a commit that referenced this pull request May 1, 2019
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>
@bors
Copy link
Contributor

bors bot commented May 1, 2019

Build succeeded

@bors bors bot merged commit 2949f15 into rust-num:master May 1, 2019
@Schultzer
Copy link
Contributor Author

@cuviper No not at all I'm here to learn, Thanks.

@Schultzer Schultzer deleted the add-mul-add-mul-add-assign branch May 1, 2019 23:47
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

Successfully merging this pull request may close these issues.

Add MulAdd
2 participants