Skip to content

Fix ToMetric() rounding boundary issue for doubles #1695#1696

Open
reabr wants to merge 2 commits into
Humanizr:mainfrom
reabr:fix/tometric-rounding-1695
Open

Fix ToMetric() rounding boundary issue for doubles #1695#1696
reabr wants to merge 2 commits into
Humanizr:mainfrom
reabr:fix/tometric-rounding-1695

Conversation

@reabr

@reabr reabr commented Mar 28, 2026

Copy link
Copy Markdown

Fixes #1695

Problem

Calling ToMetric() on a double that rounds up to a power-of-1000
boundary displays the raw number instead of the metric representation.

For example:

  • 999.9.ToMetric(decimals: 0) returns "1000" instead of "1k"
  • 999.99.ToMetric(decimals: 1) returns "1000" instead of "1k"

Root Cause

In BuildRepresentation(double, ...), the exponent is calculated from
the original input before rounding. For 999.9, the exponent evaluates
to 0, so the method skips the metric path entirely and falls through
to plain number formatting. The subsequent rounding then produces 1000
with no metric suffix.

Fix

After detecting exponent == 0, re-check the exponent on the rounded
value before falling through to plain formatting. If rounding pushed the
number across a boundary, dispatch to BuildMetricRepresentation instead.

Tests

Added inline test cases to the existing ToMetric theory in
MetricNumeralTests.cs covering the reported case and edge cases.

Comment thread src/Humanizer/MetricNumeralExtensions.cs Fixed
@reabr
reabr force-pushed the fix/tometric-rounding-1695 branch from fde266f to 64050e9 Compare March 28, 2026 15:52
git push --force origin fix/tometric-rounding-1695#	modified:   tests/Humanizer.Tests/MetricNumeralTests.cs
@reabr
reabr force-pushed the fix/tometric-rounding-1695 branch from 64050e9 to a8079ca Compare March 28, 2026 16:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an edge case in ToMetric(double, ...) where rounding can push values across a 1000^n boundary (e.g., 999.91000) but the representation logic previously stayed on the non-metric formatting path.

Changes:

  • Adjust BuildRepresentation(double, ...) to re-evaluate the metric exponent after rounding when the initial exponent is 0.
  • Add theory cases covering boundary-rounding scenarios (positive/negative, different decimal settings) to prevent regressions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Humanizer/MetricNumeralExtensions.cs Re-checks exponent based on the rounded value to correctly switch to metric formatting when rounding crosses a boundary.
tests/Humanizer.Tests/MetricNumeralTests.cs Adds coverage for rounding-to-boundary cases like 999.91k and related edge conditions.
Comments suppressed due to low confidence (1)

src/Humanizer/MetricNumeralExtensions.cs:407

  • In the exponent == 0 + decimals.HasValue path, input is rounded into rounded, but then Math.Round(input, decimals.Value) is executed again when building representation. Consider reusing the already-computed rounded for the plain-number fallback to avoid duplicated work and to ensure the same rounded value is used consistently across both the boundary check and the final formatting.
        if (decimals.HasValue)
        {
            var rounded = Math.Round(input, decimals.Value);
            if (!rounded.Equals(0d))
            {
                var roundedExponent = (int)Math.Floor(Math.Log10(Math.Abs(rounded)) / 3);
                if (!roundedExponent.Equals(0))
                    return BuildMetricRepresentation(rounded, roundedExponent, formats, decimals);
            }
        }
        var representation = decimals.HasValue
            ? Math
                .Round(input, decimals.Value)
                .ToString()

@logiclrd

Copy link
Copy Markdown
Contributor

This implementation rounds the number a second time needlessly, and repeats the scale calculation logic for what is conceptually a much simpler check:

        if (decimals.HasValue)
        {
            var rounded = Math.Round(input, decimals.Value); ////// Rounds the input
            if (!rounded.Equals(0d))
            {
                ////// Very expensive way to check if a number's absolute value is >= 1000
                var roundedExponent = (int)Math.Floor(Math.Log10(Math.Abs(rounded)) / 3);
                if (!roundedExponent.Equals(0))
                    return BuildMetricRepresentation(rounded, roundedExponent, formats, decimals);
            }
            ////// Local variable rounded is thrown away
        }
        var representation = decimals.HasValue
            ? Math
                .Round(input, decimals.Value) ////// Rounds the input again
                .ToString()
            : input.ToString();
        var space = (formats & MetricNumeralFormats.WithSpace) == MetricNumeralFormats.WithSpace ? " " : string.Empty;
        return representation + space;

Proposed alternative:

    static string BuildRepresentation(double input, MetricNumeralFormats? formats, int? decimals)
    {
        var exponent = (int)Math.Floor(Math.Log10(Math.Abs(input)) / 3);

        if (!exponent.Equals(0))
            return BuildMetricRepresentation(input, exponent, formats, decimals);

        if (decimals.HasValue)
        {
            input = Math.Round(input, decimals.Value);

            if (Math.Abs(input) >= 1000)
                return BuildMetricRepresentation(input, exponent: 1, formats, decimals);
        }

        var representation = input.ToString();
        var space = (formats & MetricNumeralFormats.WithSpace) == MetricNumeralFormats.WithSpace ? " " : string.Empty;
        return representation + space;
    }

In this version:

  • The input parameter variable stores the result of rounding.
  • The check for the overflow condition is semantic, and, for what it's worth, uses fewer CPU cycles.
  • The statement that assigns to representation is considerably simplified.

This code is conceptually simpler and easier to maintain.

I also removed extra braces around single-line blocks, because I noticed that this style is now explicitly called out in the PR checklist.

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.

Calling ToMetric() on a number that rounds up to 1000 displays 1000 instead of 1k

4 participants