Skip to content

Fix si_format() producing out-of-range mantissa like "1000.0 k"#17

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

Fix si_format() producing out-of-range mantissa like "1000.0 k"#17
patchwright wants to merge 1 commit into
cfobel:masterfrom
patchwright:fix/rounding-rollover

Conversation

@patchwright

Copy link
Copy Markdown

Description

si_format() can print a mantissa of 1000 with the smaller prefix, which is
outside the [1, 1000) range each SI prefix represents:

>>> from si_prefix import si_format
>>> si_format(999999)
'1000.0 k'         # expected: '1.0 M'
>>> si_format(999999, precision=0)
'1000 k'           # expected: '1 M'
>>> si_format(999.9, precision=0)
'1000 '            # expected: '1 k'
>>> si_format(999999999)
'1000.0 M'         # expected: '1.0 G'

Root cause

split() selects the prefix by promoting the mantissa while value >= 1000.0,
but it works on the unrounded value. si_format() then rounds the mantissa to
precision digits ("%.1f" % 999.999"1000.0"). So a value just below 1000
rounds up to 1000 after the prefix has already been chosen.

Fix

Make split()'s promotion test the value rounded to precision, so a mantissa
that will round to 1000 is carried into the next prefix:

if round(value, precision) >= 1000.0:
    value /= 1000.0
    expof10 += 3

Values below the rounding boundary (si_format(999500, 1) == "999.5 k") and
higher-precision inputs (si_format(999999, 4) == "999.9990 k") are unaffected.

Tests

  • Added test_si_format_rounding_rollover with the cases above; it fails on
    master and passes with the fix.
  • The existing 38 si_format cases are unchanged — full suite (112 tests) green.

split() chooses the SI prefix from the unrounded mantissa, but
si_format() rounds the mantissa to `precision` digits afterwards.
A value just under 1000 (e.g. 999.999 from si_format(999999)) rounds
up to 1000 and is printed with the smaller prefix -> "1000.0 k"
instead of "1.0 M".

Make split()'s >= 1000 promotion test the value rounded to
`precision`, so it carries into the next prefix. Values below the
rounding boundary and higher-precision inputs are unaffected.

Adds parametrized regression cases; the existing 38 si_format cases
are unchanged.
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