Add ApproxEq::abs_diff_{eq|ne} functions and macros #16
Conversation
|
Would like some feedback @andolon! I just copied over the tests from |
There was a problem hiding this comment.
I'm not overly familiar with your code base, but things look sensible to me. I don't see any particular problems with the tests.
I'm not sure if it's actually wise to provide a default epsilon/tolerance - this is the very problem that Bruce Dawson discusses in the randomascii blog I know we've both spent some time reading. The problem with absolute differences is that there is really no sensible default - it depends entirely on the numbers you are comparing.
| @@ -192,6 +213,11 @@ macro_rules! impl_float_approx_eq { | |||
| fn default_max_ulps() -> u32 { 4 } | |||
|
|
|||
| #[inline] | |||
| fn abs_diff_eq(&self, other: &$T, epsilon: $T) -> bool { | |||
| $T::abs(self - other) <= epsilon | |||
There was a problem hiding this comment.
If this trait is intended to be possible to be implemented for unsigned integers, then you might run into underflow problems here. In rulinalg we handle this by a conditional on which element is bigger, see here. At the moment it's not a problem though
|
Hmm, yes, I see. I guess it would be good to have some nice docs helping people understand how to choose the tolerance value. I don't really have enough experience to say though, and that was the biggest thing that stumped me with his article. I'm sure it would confuse other newcomers as well. Edit: made an issue for this at #17 |
|
At the moment all functions have a default epsilon, so I'll stick with that for this PR. But it's definitely a good point that we should consider! |
Closes #8.
The documentation could be a great deal better, especially given @Andlon's brilliantly in-depth answers on #8. We should definitely revisit it and give it the love it deserves. Other future things that might be interesting to explore is copying some design decisions from the macros provided in rulinalg.