Repository navigation
Select the compact pattern from the rounded value - #1301
Conversation
You should fix your environment. Unless, of course, this comment was maybe written by an agent in a sandbox. |
c8465ba to
e48e5a6
Compare
|
You were right, my checkout had CLDR 47 imported while master expects 48.2. I have re-run scripts/download_import_cldr.py and the full suite now passes: 6608 passed, 1166 skipped, 1 xfailed. Sorry for the misleading note, I have removed it from the description. The branch is also rebased onto current master. |
|
Hi @akx, a gentle check-in, and no rush. You were right about my environment back in July, and I wanted to make sure my answer did not get lost. My checkout had CLDR 47 imported while master expects 48.2, which is what produced the unrelated failures I had reported. I re-ran On the change itself: Happy to answer anything else, or to close it if you would rather this were solved a different way. |
|
Hi @akx, one more gentle follow-up on this, and then I will leave it be so as not to nag. Now that the environment mix-up is sorted (my checkout had CLDR 47 while master expects 48.2), this is a small, self-contained CLDR-compliance fix. The branch is still mergeable and the full suite passes (6608 passed, 1166 skipped, 1 xfailed). I am happy to rebase or reshape it however you prefer. No rush at all, I just did not want it to fall off the radar. |
format_compact_decimal chose the magnitude from the unrounded number and only rounded afterwards, so a value that rounds up into the next magnitude kept the smaller unit: 999999 rendered as "1000K" instead of "1M", and likewise "1000M" for 999999999 and "1000 thousand" in the long format. The CLDR algorithm that _get_compact_format documents selects the pattern from the rounded value, so redo the lookup one magnitude up when rounding carries over. Values that do not carry over are unaffected.
e48e5a6 to
ea619b2
Compare
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
LGTM. I unnested the loop a bit for good measure.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1301 +/- ##
==========================================
+ Coverage 93.55% 93.56% +0.01%
==========================================
Files 27 27
Lines 4931 4939 +8
==========================================
+ Hits 4613 4621 +8
Misses 318 318
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
format_compact_decimalpicks the magnitude bucket from the unrounded number and only rounds afterwards, so a value that rounds up into the next magnitude keeps the smaller unit:"1000K" is not a valid compact form, since the mantissa is supposed to stay below the next magnitude. The CLDR algorithm that
_get_compact_formatlinks in its own docstring selects the pattern from the rounded value, so this redoes the lookup one magnitude up when rounding carries over.The retry goes through
_get_compact_formatagain rather than patching the pattern in place, so the next bucket's plural form is re-selected too; that is what makes the long format read "1 million" instead of "1 millions". It terminates because the bumped value sits exactly on the magnitude boundary and cannot carry a second time.Values that do not carry over are unaffected:
999->999,1000->1K,99999->100K,9999999->10M,999949withfraction_digits=1->999.9K,12345withfraction_digits=2->12.34K. Negatives keep their sign (-999999->-1M).Added
test_compact_rounding_carries_to_next_magnitude; it fails on the current code withassert '1000K' == '1M'.