Repository navigation
refactor: add number formatting utilities for improved data presentation - #7871
Conversation
| format_token_number(_round_ratio(total_tokens, chat_record_count_total)), | ||
| ] | ||
| worksheet.append(row) | ||
| response = HttpResponse(content_type="application/vnd.ms-excel") |
There was a problem hiding this comment.
The diff is syntactically plausible, but there are several semantic/data-presentation issues. The most important ones are below.
Critical issues
-
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. - Original:
-
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.
- Original order:
-
_proportion_percentagechanges ratio semantics from 0–1 to 0–100- Original values such as
total_tokens / token_countwere ratios in the range0.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.
- Original values such as
-
Formatted numbers may become text cells in Excel
format_token_number(...)andnumber_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_numberfor ratios/averages unless that is explicitly desired. A ratio is not a token count.
Potential issues
-
Helpers do not handle
Nonesafely_round_ratio(None, 10)raises onDecimal(None)._round_ratio(10, None)raises onNone <= 0._proportion_percentage(None, total)raises onDecimal(None).
Consider guarding withvalue or 0andif divisor is None or divisor <= 0.
-
Decimal(value)can introduce binary-float artifacts
Ifvalueordivisorcan befloat, use:Decimal(str(value))
instead of
Decimal(value). For integer counts this is less important, but it is safer. -
Converting
Decimaltofloatcan lose precision
In_round_ratioand_proportion_percentage, converting theDecimalresult tofloatbefore checking.is_integer()can lose precision for very large values.
Safer pattern:if dec == dec.to_integral_value(): return int(dec) return float(dec)
-
Clamping percentages to 100 hides data anomalies
min(100.0, max(0.0, ...))silently caps invalid values. Ifvalue > totalindicates a data-quality problem, consider logging or exposing it rather than silently clamping. -
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_ratioand_proportion_percentage. - Add unit tests for:
- zero denominator,
Nonevalues,value > total,- reciprocal ratio cases,
- column order and header alignment.
- Keep numeric data numeric in Excel and use
openpyxlcell number formats for display. - Use a consistent naming convention, e.g.
_round_quotientand_percentage, since_round_ratioactually computes a rounded quotient. - Verify that
format_token_numberandnumber_formatexist in the importedcommon.utils.commonmodule 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.
refactor: add number formatting utilities for improved data presentation