Repository navigation
fix(xlsx): render float values faithfully instead of 6-digit scientific notation - #2533
Manohar Paturi (ManoharPaturi) wants to merge 4 commits into
Conversation
XU (kokokoXUY)
left a comment
XU (kokokoXUY)
left a comment
There was a problem hiding this comment.
This PR for #2532 currently contains the fenced-code whitespace change from #2529 and the DOCX heading change from #2531 as well as the XLSX float change. Both earlier PRs are still open against main. In particular, this branch still has the original fenced-code normalizer from commit b069ae1, while #2529 was updated to 7e6ed5c after review to fix the fence-boundary case. I compared the current _markitdown.py at all three PR heads: #2531 and #2533 have the same older file, and #2529 has the revised one.
Could you rebase or cherry-pick so this PR's diff against main contains only the XLSX float change and its test? That would keep the already-reviewed fence correction and the separate DOCX work from being mixed into this merge.
pandas to_html's default float format silently drops entered digits (123456789.123 -> 1.234568e+08). Pass float_format so the shortest round-tripping representation of the stored value is used. Applies to both the .xlsx and .xls paths. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
1c27828 to
383fe6d
Compare
|
I verified 383fe6d with pandas 2.3.3 and 3.0.5. The reported One small test improvement: the patch changes both XLSX and XLS conversion, but the committed regression only exercises XLSX. Could you add an XLS case and assert the |
|
Added the suggested test coverage on top of 383fe6d: 8936a6f (patch). Feel free to cherry-pick it. This adds the missing Both tests pass with pandas 2.3.3 and 3.0.5. Full core suite on Python 3.12: 968 passed, 43 skipped; Black 23.7.0 and diff whitespace checks pass. |
|
I opened #2568 for this same issue before finding this PR — I'm closing mine as a duplicate. Yours came first and the approach is the same, so this one should be the one that lands. One detail worth pinning down while you're here: I verified the behaviour locally on current
A standalone assertion for the numpy case, if it's useful: def test_float_format_handles_numpy_scalars() -> None:
"""``repr`` must not leak numpy's ``np.float64(...)`` wrapper into the output."""
import numpy as np
# or whatever the callable ends up being named
assert _format_float(np.float64(0.1)) == "0.1"Thanks! |
Add a regression proving the float() cast inside float_format is load-bearing: pandas hands the formatter numpy scalars, and a bare repr(value) renders 'np.float64(0.1)' in the table on numpy >= 2. Also reformat the xls to_html call site; the committed indentation failed the repo's black 23.7.0 pre-commit hook.
|
Çağdaş Yürekli (@cagdasyurekli) cherry-picked your commit, thanks for putting it together (b888025, your authorship kept). The XLS fixture and the 1e-10 assertion are in. woodforestsen added the regression you suggested (d362f08). Confirmed the float() cast is load bearing: pandas hands the formatter numpy scalars, and on numpy >= 2 a bare repr(value) prints np.float64(0.1) in the table. The test builds a sheet with np.float64 values through pandas and asserts the wrapper never shows up. Also fixed the indentation of the xls to_html call in d362f08. It was failing the repo's black 23.7.0 pre-commit hook, so CI would have flagged it once the workflows run. |
XU (kokokoXUY)
left a comment
XU (kokokoXUY)
left a comment
There was a problem hiding this comment.
Review: independent re-verification of the float-fidelity fix at head d362f08
I reviewed this PR earlier at 1c27828 (review 5319182919). The branch has since moved
through 383fe6d → b888025 → d362f08, so this is a fresh verification against the
current head rather than a repeat of the earlier one.
Setup: two clean worktrees at base 1f9530a and head d362f08, same interpreter
(Python 3.13.12, pandas 3.0.6, numpy 2.5.3, openpyxl 3.1.5, xlrd 2.0.2), markitdown.__file__
asserted to come from the worktree under test.
1. The reported bug, and the fix, on single cells
Each head-side cell is exactly the shortest string that round-trips the stored double.
2. The real-world impact is wider than the description suggests
The 6-digit formatting is decided per column, not per cell. A single float column holding
[0.1, 1.0, 2.5, 1234.5, 123456789.123] renders on base as
1.000000e-01 | 1.000000e+00 | 2.500000e+00 | 1.234500e+03 | 1.234568e+08
i.e. one large magnitude drags every ordinary price in that column into scientific
notation. On head the same column is 0.1 | 1.0 | 2.5 | 1234.5 | 123456789.123. That makes
the bug materially worse than "large values lose digits", and it is worth one sentence in
the PR description / release note.
3. Red/green with this PR's own tests
- head:
3 passed. - base, with
test_xlsx_float_precision.pyandtests/test_files/float_precision.xls
copied into the base worktree so the new test file can run there:2 failed, 1 passed.
The xlsx test sees| 1.234568e+08 |, the xls test sees
['value', '1.234568e+08', '1.000000e-01', '1.000000e-10']. The numpy-scalar test passes
on both, as expected for a guard rather than a regression test.
4. Suite and lint parity
packages/markitdown/tests, head: 17 failed, 993 passed, 5 skipped; base:
17 failed, 990 passed, 5 skipped (delta = the 3 new tests). The failure sets are
identical and environmental on this machine: GBK-console decoding in
test_cli_vectors.py, missingazure-ai-*intest_cu_converter.pyand
test_docintel_html.py, missingSpeechRecognitionintest_module_misc.py. None of
them touch the converters.black==23.7.0, the version pinned in.pre-commit-config.yaml: both changed Python
files are unchanged by it.- CI on
d362f08has not actually run yet:license/claissuccess, while thetests
andpre-commitworkflow runs areaction_required(fork approval), and
mergeable_state=behind.
5. What the change deliberately does not touch (identical on both sides)
float_formatonly reaches columns pandas infers as floats. Columns typedint64,
uint64orobjectare passed through untouched:1e20(object),1e16(int64),
integer-valued1.0(openpyxl writes<v>1</v>, so int64) and
9007199254740993→9007199254740992all render the same before and after. This is
pre-existing and out of scope; I mention it only so the fix is not read as covering
every numeric-fidelity case.- Blank cells and NaN still render as
NaN: pandas'na_reptakes precedence over
float_format, so no empty-cell regression. - A cell holding
1.7976931348623157e308fails on both trees with
XlsxConverter threw OverflowError: cannot convert float infinity to integer: openpyxl
reads the value back asinf, then pandas rejects it. Excel cannot store that value, so
this is reachable only with a synthetic file, and it is not caused by this PR.
6. Format changes beyond the bug fix (non-blocking, but visible)
- Decimal padding disappears:
[1.0, 2.5, 100.0, 0.25]was1.00 | 2.50 | 100.00 | 0.25
and is now1.0 | 2.5 | 100.0 | 0.25. Correct and round-trippable, but any snapshot
test or downstream consumer that expects padded currency will see a different table. - Large integral values inside float columns become
1e+16/1000000000000000.0where
base printed1.000000e+15/1.000000e+16; again faithful to the stored double.
7. Coverage limits and small suggestions (non-blocking)
- openpyxl writes only 15 significant digits —
0.1 + 0.2lands in the sheet XML as
<v>0.3</v>— so a fixture generated with openpyxl cannot exercise 17-digit
round-tripping. The new tests are perfectly valid for the reported bug (their values are
≤ 12 significant digits), they just should not be read as covering that boundary. - The
float_format=lambda value: repr(float(value))lambda is now duplicated in
XlsxConverterandXlsConverter. A module-level helper (with the numpy-scalar
reasoning as its docstring) would keep the two paths in step; today a future edit to one
call site can silently leave the other behind. - This repository has no CHANGELOG, so omitting one is correct.
8. Cost
40,000 float cells: 1.836s → 1.706s; 100,000 cells: 4.402s → 4.174s (whole-file
conversion, single machine) — no measurable regression.
Verdict: the change fixes what it claims on both the .xlsx and the .xls path, keeps
comparable sets byte-identical, passes this PR's tests, and does not regress the suite I
can run locally. From my side nothing blocks merge; the only open items are procedural
(branch is behind main, CI awaiting maintainer approval).
AI disclosure: I used an AI assistant to write the probe scripts, run the measurements and
draft this review; every number above comes from the runs described here and is
reproducible with two worktrees (1f9530a vs d362f08) and the PR's own test file.
Fixes #2532.
Passes
float_formatto bothto_htmlcall sites so a float renders as the shortest string that round-trips its stored value instead of pandas' 6-significant-digit scientific notation. Integral columns keep rendering through the existing paths (the integer formatting from #2485-style handling is untouched;42.0in a genuinely-float column renders as42.0).Regression test asserts
123456789.123survives and1.234568e+08does not appear (fails on main). Existing xlsx/xls suites green (27 passed).