Skip to content

Fix millify() producing out-of-range mantissa like "1000k" - #13

Open
patchwright wants to merge 1 commit into
azaitsev:masterfrom
patchwright:fix/millify-rounding-rollover
Open

Fix millify() producing out-of-range mantissa like "1000k"#13
patchwright wants to merge 1 commit into
azaitsev:masterfrom
patchwright:fix/millify-rounding-rollover

Conversation

@patchwright

Copy link
Copy Markdown

Description

millify() can return a mantissa of 1000 with the smaller unit suffix,
which is outside the [1, 1000) range each suffix is meant to represent:

>>> from millify import millify
>>> millify(999999)
'1000k'            # expected: '1M'
>>> millify(999500)
'1000k'            # expected: '1M'
>>> millify(999999, precision=2)
'1000k'            # expected: '1M'  (precision does not help)

Root cause

The suffix index millidx is derived from log10(abs(n)) / 3 before the
mantissa is rounded to precision digits. When the rounded value reaches 1000
(e.g. 999999 / 1000 = 999.999, which rounds to 1000), the suffix is still the
smaller one, so the output is "1000k" instead of carrying into the next unit.

Fix

After rounding, if the mantissa is >= 1000 and a larger suffix is available,
bump millidx and recompute. This keeps the mantissa in range and leaves the
top of the suffix table untouched (no overflow past "Y").

if abs(float(result)) >= 1000 and millidx < len(millnames) - 1:
    millidx += 1
    result = '{:.{precision}f}'.format(n / 10**(3 * millidx), precision=precision)

Tests

The repository had no test suite, so this adds test_millify.py (pytest) with:

  • baseline cases (0, 999, 1000, 1M, negative, precision),
  • regression cases for the rollover bug (999999/999500/-9999991M,
    9999999991B), which fail on master and pass with the fix,
  • a guard that the largest unit (Y) does not overflow.

All 12 pass with the fix.

The suffix index is chosen from log10(n) before the mantissa is
rounded to `precision`. When the rounded value reaches 1000 (e.g.
millify(999999) -> "1000k", or 999500 -> "1000k"), the result is
no longer in the [1, 1000) range the suffix represents.

Carry the value into the next unit when the rounded mantissa is
>= 1000 and a larger suffix is available, so millify(999999) -> "1M".

Adds test_millify.py with regression cases (the repo had no tests).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant