Closed olsavmic closed 1 year ago
Hi, I think this makes a lot of sense! Could you please add a test that would fail without simplify()
and succeed with it?
Hi, I added two test cases showing the issue. Seems like I was mistaken with the "Such loss of precision caused by a very large nominator or denominator may be actually (and unexpectedly) way larger..." part (and the error is always within the scale of PHP_FLOAT_EPSILON) but the test case with float overflow shows a real-world scenario that can easily occur and can be prevented. :)
Thanks, could you please avoid rewriting the coding style as it makes reading the diff tedious. FYI we’re working on imposing a coding style on the project and while some or your changes are relevant, they’ll be applied in a separate step.
Of course, sorry for that!
Calling
$rationalNumber->toFloat()
may result in a huge loss of precision due to the limits of floating point representation which can be prevented (not every time, but in a lot of real-world cases) by calling->simplified()
first.Such loss of precision caused by a very large nominator or denominator may be actually (and unexpectedly) way larger than if regular 64bit floating numbers were used for all math operations instead (assuming some real-world numeric range).Even with regular numeric range, float overflow may easily occur.We ran into this issue in production quite a few times in the legacy parts where we did not migrate yet to using BigRational and we perform the float conversion.
Therefore I'd like to suggest calling
->simplify
every time before->toFloat()
conversion. I understand that the operation does not come for free, I'd like to hear your opinion :)Thank you for this otherwise excellent library!