Skip to content

refactor: add number formatting utilities for improved data presentation - #7871

Merged
wxg0103 merged 1 commit into
v3from
pr@v3@refactor_homepage_import
Nov 12, 2026
Merged

wxg0103 merged 1 commit into
v3from
pr@v3@refactor_homepage_import

Conversation

@shaohuzhang1

Copy link
Copy Markdown
Contributor

refactor: add number formatting utilities for improved data presentation

@wxg0103
wxg0103 merged commit 7a8feca into v3 Nov 12, 2026
3 of 4 checks passed
@wxg0103
wxg0103 deleted the pr@v3@refactor_homepage_import branch November 12, 2026 09:29
format_token_number(_round_ratio(total_tokens, chat_record_count_total)),
]
worksheet.append(row)
response = HttpResponse(content_type="application/vnd.ms-excel")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The diff is syntactically plausible, but there are several semantic/data-presentation issues. The most important ones are below.

Critical issues

  1. The second export reverses a ratio formula

    • Original:
      item.chat_user_count / item.chat_record_count_total if item.chat_record_count_total != 0 else 0
    • New:
      _round_ratio(item.chat_record_count_total, item.chat_user_count)

    This now computes chat_record_count_total / chat_user_count, which is the reciprocal of the original metric. If the original metric was intentional, this is a bug. If the intended metric is “records per user”, then the Excel header and any downstream consumers must also be changed.

  2. The third export swaps two columns

    • Original order:
      item.chat_user_count,
      chat_record_count_total,
    • New order:
      number_format(chat_record_count_total),
      number_format(item.chat_user_count),

    Unless the Excel headers were also swapped in a part of the file not shown in this diff, the exported columns will no longer match their headers. This is likely a data-alignment bug.

  3. _proportion_percentage changes ratio semantics from 0–1 to 0–100

    • Original values such as total_tokens / token_count were ratios in the range 0.0–1.0.
    • New helper returns percentages in the range 0–100, rounded to one decimal.
      This may be intentional, but it requires the Excel headers and any consumers of the export to be updated to indicate “%” or “percentage”. Otherwise the meaning of the column silently changes.
  4. Formatted numbers may become text cells in Excel

    • format_token_number(...) and number_format(...) likely return strings such as "1,234" or "1.2K".
    • If appended to the worksheet as strings, Excel will treat those cells as text, not numbers. That breaks sorting, filtering, summing, formulas, and numeric analysis.
    • Preferred approach: append numeric values and set the cell display format, e.g.:
      worksheet.append([... numeric values ...])
      cell.number_format = "#,##0"
    • Also avoid using format_token_number for ratios/averages unless that is explicitly desired. A ratio is not a token count.

Potential issues

  1. Helpers do not handle None safely

    • _round_ratio(None, 10) raises on Decimal(None).
    • _round_ratio(10, None) raises on None <= 0.
    • _proportion_percentage(None, total) raises on Decimal(None).
      Consider guarding with value or 0 and if divisor is None or divisor <= 0.
  2. Decimal(value) can introduce binary-float artifacts
    If value or divisor can be float, use:

    Decimal(str(value))

    instead of Decimal(value). For integer counts this is less important, but it is safer.

  3. Converting Decimal to float can lose precision
    In _round_ratio and _proportion_percentage, converting the Decimal result to float before checking .is_integer() can lose precision for very large values.
    Safer pattern:

    if dec == dec.to_integral_value():
        return int(dec)
    return float(dec)
  4. Clamping percentages to 100 hides data anomalies
    min(100.0, max(0.0, ...)) silently caps invalid values. If value > total indicates a data-quality problem, consider logging or exposing it rather than silently clamping.

  5. Rounding before export can make percentages not sum to 100%
    This is often acceptable for display, but if exact totals are needed, keep numeric values and round only at presentation time.

Optimization suggestions

  • Add type hints to _round_ratio and _proportion_percentage.
  • Add unit tests for:
    • zero denominator,
    • None values,
    • value > total,
    • reciprocal ratio cases,
    • column order and header alignment.
  • Keep numeric data numeric in Excel and use openpyxl cell number formats for display.
  • Use a consistent naming convention, e.g. _round_quotient and _percentage, since _round_ratio actually computes a rounded quotient.
  • Verify that format_token_number and number_format exist in the imported common.utils.common module for all supported branches/versions.

Suggested fixes

For the second export, if the original metric was intended:

number_format(_round_ratio(item.chat_user_count, item.chat_record_count_total))

For the third export, restore the original column order:

number_format(item.chat_user_count),
number_format(chat_record_count_total),

For percentage columns, either update the headers to indicate %, or change the helper back to a ratio:

# returns 0.0–1.0 ratio
def _proportion_ratio(value, total):
    if not total:
        return 0
    dec = (Decimal(value) / Decimal(total)).quantize(
        Decimal("0.001"), rounding=ROUND_HALF_UP
    )
    return int(dec) if dec == dec.to_integral_value() else float(dec)

Overall: the main risks are the inverted formula, the swapped columns, the ratio-to-percentage semantic change, and the conversion of numeric Excel cells into formatted strings.

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.

2 participants