Float-precision-aware cell change detection in the data-diff grid (DRC-3025)#1458
Closed
danyelf wants to merge 7 commits into
Closed
Float-precision-aware cell change detection in the data-diff grid (DRC-3025)#1458danyelf wants to merge 7 commits into
danyelf wants to merge 7 commits into
Conversation
Profile diff flagged numeric cells that differed only in float noise (e.g. 3.14159000001 vs 3.14159). Route all three comparison sites through one shared helper isCellChanged(base, current, columnType?, renderMode?): - formatted modes compare at one digit BELOW displayed precision (N+1) - raw mode applies a relative epsilon (FLOAT_RELATIVE_EPSILON = 1e-9) - non-numeric keeps exact _.isEqual; one-sided null/NaN counts as changed Wired getCellClass (was !_.isEqual), determineRowStatus (was !_.isEqual), and inlineRenderCell (was ===). getCellClass/determineRowStatus gained optional render-mode params (backward compatible). +32 TDD tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
…3025) Rework the float-precision fix to a simpler, corrected approach per Danyel's review. These are floating-point representation errors, so drop the display-precision-relative comparison entirely and use one flat tolerance in ALL render modes. - Simplify isCellChanged(base, current) — no columnType / renderMode params: two finite numbers compare with |a-b| <= EPS*max(|a|,|b|) (EPS=1e-9) plus a 1e-12 absolute floor near zero; everything else keeps exact _.isEqual. - Revert getCellClass and determineRowStatus to their original signatures (remove the columnRenderMode threading). This dissolves the cloud-side open question — the change is now self-contained in OSS. - Remove displayedDecimals / toComparableNumber / numericChanged / the NUMERIC_COLUMN_TYPES set. - inlineRenderCell stays wired to the helper. - Tests rewritten to render-mode-independent cases (0.1+0.2 vs 0.3, near-zero floor, large-magnitude scaling, non-numeric exact). Removed the old precision-relative / render-mode tests. Full @datarecce/ui suite green (4032 passed / 5 skipped), type:check + Biome lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
Wire the float-precision-aware isCellChanged comparison into the live diff-grid render path, which previously still used raw _.isEqual: - detectModifications (rowBuilders.ts) drives row __status="modified", rowStats.modified, and the changedOnly filter for BOTH inline and side-by-side modes. - createCellClassBase/createCellClassCurrent (toDiffColumn.tsx) drive side-by-side red/green cell highlighting. Float-noise-only rows (e.g. 0.1+0.2 vs 0.3) no longer read as modified or get highlighted. Adds tests over both surfaces with float-noise inputs. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
Rework isCellChanged onto _.isEqualWith with a floatComparator customizer so the magnitude-relative epsilon is applied at every numeric leaf, including floats nested inside object/array cells (JSON/ARRAY/STRUCT columns). The old top-level typeof===number gate only tolerated scalar float noise and compared nested floats exactly, leaving the noise bug live for structured cells. Internalize FLOAT_RELATIVE_EPSILON: the customizer closes over it, so it is now module-private -- drop its export in gridUtils.ts and remove it from both barrels (utils/dataGrid/index.ts, utils/index.ts). It had zero consumers. Add nested-float regression tests: object/array/deeply-nested cells differing only by float noise read unchanged, genuine nested and non-numeric changes still read changed. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
…oat diff (DRC-3025) Anchors the fix on the DRC's ACTUAL reported scenario, not just the 0.1+0.2-vs-0.3 scalar proxy: a profile_diff of a table that did not change must not surface phantom 'modified' rows when a stat column (avg/mean) differs only in its raw floating-point tail. Drives the real production pipeline end to end via createDataGrid: profile_diff -> toDataDiffGrid -> buildDiffRows -> isCellChanged. A float-noise-only stat row reads __status !== 'modified' in both inline and side-by-side modes; a genuine stat change still reads modified (control). Raw stat values are compared, so display precision (2 vs 5 decimals) is irrelevant by construction. Verified test-first: reverting isCellChanged to raw _.isEqual turns the two noise assertions red (row reads 'modified'); the isEqualWith fix turns them green. Full suite green (4085). Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
…how no phantom float diff (DRC-3025) The DRC's real scenario: a profile diff of a table that did NOT change flags an avg cell as modified even though both sides display the same value. Root cause is in the BACKEND, not the grid: agate NUMBER aggregates arrive as Decimal, and from_agate kept finite Decimals as-is; pydantic v2 then serializes Decimal as a JSON *string*. So the stat reached the frontend as "0.30000000000000004" vs "0.3" — two unequal strings — and the grid's numeric epsilon (isCellChanged) only tolerates float noise between NUMBERS, so the row read modified. A numeric aggregate arriving as a string is itself the bug, so fix it at the earliest correct layer: from_agate now coerces finite Decimal values in NUMBER-typed columns to float, so the stat is a real number end-to-end and the epsilon applies naturally. These stats are shown at 2-5 decimals, so float64 is the correct display-precision wire type. INTEGER columns are intentionally left untouched (already ints; exact-compare, no float noise; avoids >2**53 precision loss). Non-finite Decimals still coerce to float (GitHub #476), unchanged. Tests: - Backend (test_dataframe_number_serialization.py): NUMBER Decimals serialize as JSON numbers; a 0.1+0.2 vs 0.3 pair is numerically within epsilon; genuine change still differs; non-finite still float; INTEGER unaffected. Proven test-first (reverting the coercion turns the NUMBER cases red with str values). - Frontend (createDataGrid.profile.test.tsx): re-anchored on both wire shapes — the post-fix NUMBER shape reads UNCHANGED through the real createDataGrid -> toDataDiffGrid -> isCellChanged path (inline + side-by-side), and the pre-fix STRING shape still reads MODIFIED, documenting why the fix must live in the backend (the frontend does not numerically compare stringified numbers). Full suites green: backend 1552 passed/5 skipped; frontend 4085 passed/5 skipped. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
…ta exact (DRC-3025) Cycle-4 put the NUMBER Decimal->float coercion in the SHARED DataFrame.from_agate, which also feeds query/query_diff/value_diff. There a NUMBER column is a raw warehouse DECIMAL/NUMERIC *data* value, so coercing to float64 masked genuine high-precision changes: e.g. 12345678901234567.89 vs ...90 collapse to the same float and read UNCHANGED (a real change silently hidden + the displayed value corrupted), where pre-fix they serialized as distinct exact strings and read MODIFIED. That is a false-negative on the core data-diff function. Fix: restore from_agate to its exact behavior (finite Decimals kept as-is; only non-finite -> float per GitHub #476), and move the coercion into the profile path (recce/tasks/profile.py "_coerce_number_stats_to_float"), applied to all three profile build sites. Only profile stats — computed aggregates where float representation noise is meaningless — become floats; query/query_diff/value_diff NUMBER data keeps its exact Decimal value and compares exactly, exactly as before cycle-4. INTEGER-exact and non-finite/#476 behavior preserved. Tests: - Backend (test_dataframe_number_serialization.py): profile stats serialize as JSON numbers via the profile helper (float-noise within epsilon; genuine change differs; non-finite float; INTEGER untouched) AND a high-precision DECIMAL data change stays distinct exact strings through from_agate (reads MODIFIED, not masked). Regression guard proven test-first: re-adding the coercion to from_agate turns the two data-exactness tests RED while profile tests stay green. - Frontend: comment re-pointed to the profile path; assertions unchanged. End-to-end seam re-verified both ways: unchanged-table profile avg -> UNCHANGED; high-precision data change -> MODIFIED. Suites green: backend 1554 passed/5 skipped; frontend 4085 passed/5 skipped. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
Contributor
Author
|
Closing without merge. DRC-3025 has been rediagnosed: the root cause is that numeric Decimals serialize to the frontend as untyped JSON strings, not the profile float-noise this PR addressed. The profile-scoped float coercion here is superseded by a new entity that fixes numeric type serialization at the DataFrame layer (see the reframed DRC-3025 description). The epsilon-comparison logic will be migrated forward into that work. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The data-diff grid no longer flags float-precision noise like
0.1+0.2vs0.3as a change — including floats nested inside JSON/array/struct cells — so users stop chasing phantom diffs.What changed
isCellChangedon lodash_.isEqualWithwith a float-leaf comparator: a magnitude-relative epsilon applied at every numeric leaf, scalar and nested.changedOnlyfilter, and side-by-side cell highlighting — through it, replacing raw_.isEqual.Evidence
Review guidance
isCellChangedis the single source of truth for value diffs; the epsilon is a private leaf comparator, no longer exported.Audit: DRC-3025 Float-Precision Rework · entity
fg(tracked in local spacedock docs)Closes DRC-3025