Fix allocation remainders for large amounts - #832
kadyrbekovhamit-cyber wants to merge 3 commits into
Conversation
Signed-off-by: kadyrbekovhamit-cyber <288885044+kadyrbekovhamit-cyber@users.noreply.github.com>
frederikbosch
left a comment
There was a problem hiding this comment.
Some remarks on the number of loops. I like the fact that we do not use float anymore. But I also wonder if the behavioral change - e.g. last ratio now gets larger proportion - is preventable.
| throw new InvalidArgumentException('Cannot allocate to none, ratio must be zero or positive'); | ||
| } | ||
|
|
||
| $ratio = is_float($ratio) ? Number::fromFloat($ratio) : Number::fromNumber($ratio); |
There was a problem hiding this comment.
Can't we modify $ratios here and prevent another loop with array_map afterwards.
There was a problem hiding this comment.
The common decimal scale is only known after all ratios have been inspected, so one additional pass is unavoidable. I now parse each ratio only once in the first loop, then normalize and sum in the second pass; the array_map call and repeated Number conversion are gone.
| } | ||
|
|
||
| $total = '0'; | ||
| $ratios = array_map(static function (float|int $ratio) use ($scale): string { |
There was a problem hiding this comment.
This loop should be merged into the former loop.
There was a problem hiding this comment.
Done in 40bfb1e: normalization and total calculation now share one loop, reducing the three preprocessing passes to two.
| return ltrim($normalized, '0') ?: '0'; | ||
| }, $ratios); | ||
|
|
||
| foreach ($ratios as $ratio) { |
There was a problem hiding this comment.
This total calculation should be merged in the earlier loop. And then checked.
There was a problem hiding this comment.
Done. Each normalized ratio is added to the total immediately in the normalization loop, and the total is checked directly afterwards.
Signed-off-by: kadyrbekovhamit-cyber <288885044+kadyrbekovhamit-cyber@users.noreply.github.com>
|
Thank you for the review. I pushed commit 40bfb1e to reduce preprocessing from three passes to two and added a compatibility regression for amount 4 with ratios 1:2. The last entry receiving the extra unit is not new behavior when its exact fractional remainder is larger; the previous implementation already returns 1 and 3 for that ordinary case. For amount 7000000000000000, the exact residual numerators are 1 and 2, so the second ratio must receive the remaining minor unit. Binary64 collapses that distinction and incorrectly gives it to the first ratio. Preserving that particular old result would preserve the numerical defect. Exact ties still keep input order and award the first entry. |
Signed-off-by: kadyrbekovhamit-cyber <288885044+kadyrbekovhamit-cyber@users.noreply.github.com>
|
Hello Frederik, Thank you again for the review. The loop changes and compatibility regression are in the PR, and GitHub currently shows all 15 checks passing. I would be glad to work through any remaining concerns about allocation behavior. The independently checkable report and evidence are here: https://www.gero.uz/research/articles/moneyphp-largest-remainder-float-collapse.html I would also welcome ongoing collaboration with MoneyPHP. My background is applied mathematics and mechanics, and I can help with exact monetary calculations, rounding conventions and regression checks across calculators. I am available remotely and would be happy to start with a small agreed task. Xamit Kadirbekov / GERO |
Summary
Avoid converting allocation shares to PHP
floatwhen ranking largest remainders.For example:
Before this change the result is:
The exact largest-remainder result is:
At this magnitude both fractional shares collapse to zero in binary64, so the residual unit is assigned to the first ratio rather than the larger exact remainder.
Change
CalculatorValidation
BcMathCalculatorandGmpCalculatorphp -landgit diff --checkpassThis is an ordinary numerical-correctness fix; the allocated total was conserved before the patch, but the wrong recipient could receive the residual minor unit.