Carry metric() to the next SI prefix when rounding reaches 1000#328
Open
gaoflow wants to merge 1 commit into
Open
Carry metric() to the next SI prefix when rounding reaches 1000#328gaoflow wants to merge 1 commit into
gaoflow wants to merge 1 commit into
Conversation
metric() chose the SI prefix from floor(log10(value)) on the raw value and then rounded the mantissa to the requested precision. For a value just below a power-of-1000 boundary the rounded mantissa became 1000 while the prefix stayed put, so metric(999.9, "V") returned "1000 V" and metric(999999, "V") returned "1000 kV". That is exactly the "1230K"-style output the function's docstring promises to avoid. After rounding, if the mantissa reaches 1000 and a higher prefix is available (exponent < 30), advance to the next prefix bucket and re-divide. The top of the SI range (quetta) has no higher prefix, so it is left unchanged. Output for values not on a rounding boundary is unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
metric()can emit a four-digit mantissa with a too-small SI prefix:This contradicts the function's own docstring, which states the prefix is
1000 V/1000 kVare precisely the1230K-style outputs it promises to avoid, so the documented contract is the oracle here.Root cause
metric()selects the SI prefix bucket fromexponent = floor(log10(abs(value)))on the raw value, then rounds the mantissa to the requested precision. When the value sits just below a power-of-1000 boundary, the rounded mantissa becomes1000, but the prefix was already locked in from the pre-rounding exponent, and the code never re-checks whether rounding crossed into the next bucket.Fix
After computing the mantissa and its display precision, if
round(abs(mantissa), digits) >= 1000and a higher prefix is available (exponent < 30), advance to the next prefix bucket and re-divide. The top of the SI range (quetta,Q, 10^30) has no higher prefix, so theexponent < 30guard leaves it unchanged rather than indexing past the prefix table.Tests
Added four cases to the
test_metricparametrization (the carry cases above). They fail onmain('1000 V' != '1.00 kV', etc.) and pass with this change. The fulltests/test_number.pysuite passes (223 passed), so existingmetric()output, including the high-rangeQF/qAcases and the exact power-of-1000 inputs, is unchanged.Disclosure: I developed this fix with AI assistance, under my direction. I reviewed and verified the boundary behaviour (positive carry, the negative-exponent
mVpath, and the top-of-range quetta guard) and the regression run myself, and I am happy to adjust the approach.