feat: add cross-precision mul/div(_assign)#17
Conversation
| fn mul(self, rhs: Decimal<I, D_RHS>) -> Self::Output { | ||
| Decimal(I::full_mul_div(self.0, rhs.0, <I as Cheats<D_RHS>>::SCALING_FACTOR)) | ||
| } |
There was a problem hiding this comment.
I'm concerned about the following case:
let a: Decimal<u64, 9> = Decimal::from_scaled(1);
let b: Decimal<u64, 1> = Decimal::from_scaled(1);
assert_eq!(a * b, b * a); // Fails, does not commutea * b = full_mul_div(a, b, 10);
b * a = full_mul_div(b, a, 1e9);
// substituting a = 1e-9; b = 1.0
a * b = full_mul_div(1, 10, 10); = 10 = 1.0
b * a = full_mul_div(10, 1, 1e9): = 0 = 0.0
Thus now depending on the order of operations you will get a different mathematical result, which feels quite dangerous. A slightly better approach would take the max of the two to avoid the operator precedence issue. However, I'm not sure if you can derive a compile time constant C from constants A and B?
We could just take precision = A::PRECISION + B::PRECISION and then solve for GCD precision, this was roughly what I was trying to solve in the dyn-decimal branch.
There was a problem hiding this comment.
Yeah, I'm not sure about the design yet either, hence I created it as draft PR.
I don't think returning D + D_RHS is an option. If you've Dec<i64, 12> & Dec<i64, 8>, then you can't really return Dec<i64, 20> as it will be all decimals.
One thing I could do is add a const_assert for D >= D_RHS. That way it would be harder to mess up although the errors might not be pretty.
Ideally, you would want the target precision to be decided by the user as well, but I don't think it is possible to express it as a trait (uncovered generic issue). But you can define a function like this where you can specify the target precision too.
One more option is to use the typenum crate, which allows you to do arithmetic on generics like this. But that forces using U8 instead of 8 & so on.
There was a problem hiding this comment.
I think the const_assert that D >= D_RHS is not a bad solution as it forces safe usage at compile time. Looking back I don't think we need D + D_RHS, just max(D + D_RHS).
TODO: add tests
Fixes #10.