Add general Rem and Num implementations for Complex<T> #327
Conversation
|
Just FYI it's going to be a week or so before I can review this in depth. |
complex/src/lib.rs
Outdated
| type FromStrRadixErr = ParseComplexError<T::FromStrRadixErr>; | ||
|
|
||
| /// Parses `a +/- bi`; `ai +/- b`; `a`; or `bi` where `a` and `b` are of type `T` | ||
| fn from_str_radix(s: &str, radix: u32) -> Result<Self, Self::FromStrRadixErr> |
There was a problem hiding this comment.
This is identical to the from_str implementation, apart from nested calls to from_str_radix instead, right? Let's see if we can use a common helper function instead, something like:
fn from_str_any<T, E, F: Fn(&str) -> Result<T, E>>(s: &str, from: F) -> Result<Complex<T>, E> { ... }from_strcallsfrom_str_any(s, T::from_str)from_str_radixcallsfrom_str_any(s, |s| T::from_str_radix(s, radix))
complex/src/lib.rs
Outdated
| let neg = zero.clone() - one.clone(); | ||
| // Traverse the 3x3 square of gaussian integers surrounding our | ||
| // current approximation, and select the one whose product with | ||
| // `modulus` is closest to `self`. |
There was a problem hiding this comment.
So you're basically rounding the division to the nearest gaussian? I think this is makes it overly complicated, whereas simply rounding towards zero will at least agree with pure-real Rem.
e.g. 2.0 % 3.0 == 2.0, but in Complex your implementation gives -1.0.
There was a problem hiding this comment.
The funny thing about rounding towards zero is that you can end up with remainders that have larger magnitude than your modulus. For instance you'd have (3 + 3i) % 4 == 3 + 3i, which has greater magnitude than 4.
Of course if we go with lexicographical ordering of complex numbers, similar issues might be unavoidable. I'm not sure there is any sensible scheme where the remainder is never lexicographically greater than the modulus.
There was a problem hiding this comment.
Yes, that is a funny example. Do you have one that would fail lexicographical ordering?
I guess we see why ordering and remainders aren't usually implemented for complex numbers. But if we do implement them, I think it's a fundamental property that operations on Complex values with zero imaginary should match the equivalent operation on the real part alone.
There was a problem hiding this comment.
So one example is (1+5i) % 2i: your quotient is 2.5-.5i, which rounds to 2, and then your remainder is 1+1i, which is lexicographically larger than your modulus.
Are you aware of any compiler warnings along the line of #[deprecated] that would be appropriate for functions that may have unexpected behavior? And what would you think of applying them to rem and partial_cmp?
There was a problem hiding this comment.
P.S. I thing having complex numbers with zero imaginary component try to match behavior with real numbers is a fine principle; just pointing out places where it can get weird.
complex/src/lib.rs
Outdated
| @@ -758,6 +810,15 @@ impl<T: Clone + Num> Div<T> for Complex<T> { | |||
| } | |||
| } | |||
|
|
|||
| impl<T: Clone + Num + PartialOrd> Rem<T> for Complex<T> { | |||
There was a problem hiding this comment.
I think now this doesn't need PartialOrd, right? And we can revert the related macro changes.
There was a problem hiding this comment.
Yes, good point.
|
bors r+ |
327: Add general Rem and Num implementations for Complex<T> r=cuviper a=carrutstick This should address #209 with eyes towards addressing #321. It was a little tricky to get `Rem` working for a general `Num`, and I had to add a `PartialOrd` constraint to get it working, but I think it should be fairly robust. I could probably use extra eyes on the `from_str_radix` function, as I mostly lifted the code from the `from_str` function and I may be missing some subtleties in how that works.
Build succeeded |
This should address #209 with eyes towards addressing #321.
It was a little tricky to get
Remworking for a generalNum, and I had to add aPartialOrdconstraint to get it working, but I think it should be fairly robust.I could probably use extra eyes on the
from_str_radixfunction, as I mostly lifted the code from thefrom_strfunction and I may be missing some subtleties in how that works.