Implement arithmetic with real for complex

[IvanUkhov]

Hello,

It might be handy to be able to perform basic arithmetic operations in expressions mixing complex numbers and numeric primitives; see #116. I would be grateful for any feedback, especially regarding the subsets of primitives for which certain operations are implemented.

Regards,
Ivan

[IvanUkhov]

Following the code that was already written, I have added the forwarding of value-value, value-reference, and reference-value implementations to the corresponding reference-reference implementations.

[cuviper]

Why does this need to explicitly impl primitives? Shouldn't we do this for any generic T? Like:

// (a + i b) + c == (a + c) + i b
impl<'a, 'b, T: Clone + Num> Add<&'b T> for &'a Complex<T> {
    type Output = Complex<T>;

    #[inline]
    fn add(self, other: &T) -> Complex<T> {
        Complex::new(self.re.clone() + other.clone(),
                     self.im.clone())
    }
}

I do want that comment showing the intended math, even for simple cases like this one. Then repeat this for swapped order, of course, and I think all the forwarding can be extended in the existing macros.

How does that sound?

[IvanUkhov]

Thanks for the feedback, Josh!

The operations wherein real numbers are on the right-hand side are implemented for any T, and your example is one of such cases (what you wrote is exactly what I have). However, the same generality can’t be attained when working from the left-hand side. That is why I implemented them for concrete types. If you know how to do this for generic types, please let me know.

I agree about explanatory comments.

Regarding macros, the above from-the-left-side problem made macros quite dissimilar, and I thought that a separate set of macros would be better for understanding. In any case, I can surely try to extend and reuse the macros that are already there.

Please let me know what you think. Thanks.

[cuviper]

Argh, sorry - I didn't fully understand what you were doing, and didn't try the complete expansion myself. Now I see it falls afoul of RFC 1023, although I think we're not actually in danger of the sort of orphans that is protecting against. Hmm...

This works, but I don't really like it:

pub struct Real<T>(T);

// a + (b + i c) == (a + b) + i c
impl<'a, 'b, T: Clone + Num> Add<&'b Complex<T>> for &'a Real<T> {
    type Output = Complex<T>;

    #[inline]
    fn add(self, other: &Complex<T>) -> Complex<T> {
        Complex::new(self.0.clone() + other.re.clone(),
                     other.im.clone())
    }
}

Alternatively for Real<&'a T> works too. But either way you'd have to explicitly wrap the Real in your expressions, which isn't very nice. You can also add bare primitive impls alongside this. And we'd probably want impl Op<Real<T>> for Complex<T> variants for symmetry too, even though bare T is fine in that case. So this train of thought is getting silly...

[cuviper]

To your original question about implementing subsets of primitives -- I would include unsigned types for Sub and Div too, if only because the fully Complex<T> ops are not picky about this. It won't always produce negative values, so let rust's normal runtime checking detect underflow as usual.

e.g. "a - (b + i c) -> (a - b) + i (0 - c)", where the explicit 0 makes it subtraction instead of unary negation. If a>=b and c==0, this will be fine.

For any of these real ops, we should get the exact same result as if we'd converted to Complex first.

Alternatively to all of this, we could just impl From<T> for Complex<T> as a real, to make the conversions explicit but slightly easier. Then we don't have to worry about inconsistent implementations depending on T being a primitive. Anyway, I think it's not very common in Rust to implement ops between different types, apart from ref/value.

@hauleth any thoughts here?

[IvanUkhov]

I’ve implemented From for T and &T and utilized it accordingly. Now all four operations are implemented for all numeric primitives.

Do you still think I should try to reuse forward_val_val_binop and friends?

[IvanUkhov]

I’m also a bit concerned with the impact of this forwarding business on performance. I have a very vague idea of what the compiler is doing under the hood, but I hope you carefully considered this when opting for forwarding everything to reference-reference implementations.

[hauleth]

I think that we should implement just impl<T: Add<Output=T> + Num + Clone> Add<T> for Complex<T>. This should satisfy all our needs (and this works).

[cuviper]

I would implement From the other way. Let From<T> build the new Complex directly, then From<&T> can call from(re.clone()). And actually I was suggesting From to forgo implementing Complex+real ops at all, just making it a little easier for the caller to explicitly convert. But if we keep these ops, it is nice to reuse the fully complex implementations.

I think reusing the forwarding macros makes things easier only if we stick to fully generic implementations. If we keep the special primitive treatment, then special macros are needed.

Can you elaborate your performance concern of forwarding?

[IvanUkhov]

I’ve changed the implementations of From as you suggested.

I also like that the fully complex implementations are reused. It eliminates the inconsistency of implementations that you brought up previously.

Regarding performance, I think the most common scenario is that one works with numbers by value, not by reference. In such cases, the forwarding is always exercised, and it appears as an extra function call, which introduces some undesired overhead. Of course, there is inlining, which is supposed to take case of that. I just hope that this functionality works as it is supposed to.

[cuviper]

Given that T:Num only provides by-value ops, it probably makes more sense to implement Complex<T> ops by value, and forward the various ref versions with cloning, rather than forcing the clones everywhere in the final implementation.

[IvanUkhov]

The reason I forward everything to reference-reference implementations is that I followed the code that was already there (in complex.rs). I thought there was a deep reason for this, and I also tried to keep things consistent. If you think forwarding to value-value is better, I can rewrite. Or, there could be a separate pull request changing the forwarding in the whole complex.rs.

[cuviper]

Right, I meant forwarding to value-value is probably better for everything in complex.rs. Reference ops do make sense for something like BigInt, but here it's just forcing a lot clones.

[IvanUkhov]

I’ve switched to forwarding to value-value implementations.

[cuviper]

@homu r+

[homu]

📌 Commit b57e4a4 has been approved by cuviper

[homu]

⌛ Testing commit b57e4a4 with merge 879f2c8...

[homu]

☀️ Test successful - status