VV/BUG: Compute Feature Sizes Full V&V + precision and 2D area fixes

[nyoungbq]

Compute Feature Sizes completely V&V

The patched bug was in the Kahan Summation during the reduction phase of the RectGridGeom path

[imikejackson]

Review — Actionable Items

The Kahan post-reduction fix in ComputeFeatureSizes.cpp is correct (the old code had the accumulator/input operands swapped, so the compensation term tracked error against the wrong operand). The items below are concentrated in the V&V documents, test coverage, and a few open questions. Ordered most to least important.

Must address before sign-off

• Reconcile the contradictory RectGrid feature-1 values. The report (vv/ComputeFeatureSizesFilter.md) and the provenance "expected outputs" block (vv/provenance/ComputeFeatureSizesFilter.md) state Volume = 2358.834 / ED = 16.516, but the test asserts 2362.43384f / 16.5241966f (test/ComputeFeatureSizesTest.cpp:271, :276) and the provenance's own per-slice trace sums feature 1 to 2362.434 (1119.66 + 260 + 959.616 + 23.158). The 2358.834 value is wrong/stale. Fix it in the report, the provenance summary block, and the stale code comment at test/ComputeFeatureSizesTest.cpp:228 (the comment at :269 already says 2362.434).
• Fix feature-3 ED in the provenance. It lists 3.0994, but the test asserts 3.0668866f (:278) and the stated volume of 15.104 yields ≈3.067. Correct the provenance value.
• Resolve the COMPLETE status vs. open Bug-1. The report is marked Status: COMPLETE while listing Bug-1 (2D area uses all three spacings, ComputeFeatureSizes.cpp:146) as open. Either downgrade the status or close out the bug.

Clarifications needed

• Bug-1 is a genuine deviation from 6.5.171 — and legacy is correct. Legacy FindSizes::findSizesImage computes the 2D area from the two non-flat resolutions only (e.g. Z flat → area = xRes * yRes), whereas NX uses spacing[0]*spacing[1]*spacing[2] (all three). They agree in the current test only because the fixture's flat-dim spacing is 1.0. This should be tracked as a real deviation/bug, not a precision footnote.
• The 2D oracle cannot detect Bug-1. Create2DImageDataStructure() uses spacing {20.2, 0.1, 1.0} — the flat dimension's spacing is 1.0, so multiplying by it is a no-op. If Bug-1 is meant to be guarded, add a fixture with flat-dim spacing ≠ 1.0.
• Was an A/B comparison against a real legacy build ever run? The deviation doc states the comparison was "Source-inspection comparison" and closes with "If a future comparison run against DREAM3D 6.5.171 output reveals additional deviations…", which reads as though D1 was derived analytically and never validated against actual legacy output. Please confirm. (Legacy findSizesUnstructured accumulates per-feature volumes as naive float32 += float32; the deviation is real, but a confirming legacy run would make D1 definitive rather than asserted.)

Lower priority

• Removed legacy cross-validation. Deleting the "Legacy: Small IN100 Test" removes the only test comparing against legacy-format (6_6_stats_test_v2.dream3d) reference arrays and the only large-dataset path. Consistent with the move to Class-1 inlined oracles, but worth a note in the report given there is now no automated legacy regression.
• Fragile tolerance. std::abs(volumes.getValue(1) - 2362.43384f) < std::numeric_limits<float32>::epsilon() (:271) uses float32 epsilon (~1.19e-7) as an absolute tolerance against a magnitude of ~2362, where one float32 ULP is ≈2.7e-4. This demands bit-exact equality and depends on Kahan producing bit-identical results across platforms and reduction orderings. Consider a relative tolerance.

[imikejackson]

See comments

[imikejackson]

A/B verification run — deviation report updated (D1 split into D1 + D2)

Followed up on the earlier question about whether D1 was ever validated against a real legacy build. It now has been, and the run changed the conclusion: the legacy-vs-SIMPLNX RectGrid difference has two independent precision sources, not one.

What was run

The exact CreateRectGridDataStructure() unit-test fixture (4×4×4, non-uniform bounds) was authored as a shared legacy .dream3d and run through three binaries on identical input:

• stock 6.5.171 (FindSizes)
• DREAM3D-NX (ComputeFeatureSizes)
• a 6.5.172 proof-patch build of FindSizes

Result

Array	stock-171 vs NX	6.5.172 proof-patch vs NX
Volumes	DIFFERS (~1 float32 ULP on this fixture; grows with N)	bit-identical
EquivalentDiameters	DIFFERS	bit-identical

Why the report needed updating

The published deviation file documented only D1 (float64 + Kahan summation → Volumes) and treated the EquivalentDiameters difference as merely proportional to the volume shift. The A/B run disproved that: after patching legacy so Volumes was bit-identical, EquivalentDiameters still differed by ~1 ULP. That residual is a separate deviation:

• D1 (Volumes) — legacy accumulates per-cell sizes naively in float32; SIMPLNX uses float64 + Kahan.
• D2 (EquivalentDiameters) — legacy takes the cube root via powf() on the float32-rounded volume; SIMPLNX uses std::cbrt() on the float64 volume. Independent of D1.

Only when both changes were applied to legacy did all arrays match SIMPLNX bit-for-bit — so both are required to fully explain the difference. The same powf/sqrtf-on-float32 pattern exists on the ImageGeom path, which the report previously left as an un-flagged "non-deviation"; it's now captured by D2.

Changes prepared

• V&V report + deviations/ComputeFeatureSizesFilter.md: added D2, marked D1/D2 as A/B-verified (2026-06-27), corrected the ESD text (commit db4ce0c0d on this branch, ready to push).
• The minimal two-part proof patch lives on the internal DREAM3D v6_5_172 branch (not for release) — it exists only to prove the root cause, per our policy of documenting deviations against the 6.5.171 baseline.

[imikejackson]

Pushed a review-fix commit to this branch (vv/find_feature_sizes) addressing the adversarial review. Summary:

• Fixed Bug-1 (the blocker): the 2D area formula multiplied all three spacings (a volume, not an area) and diverged from 6.5.171 whenever the flat dimension's spacing ≠ 1.0. It now uses only the two non-flat spacings. With this fix the deviations file's "ImageGeom path — no deviations" is accurate again, and the report's "1 open bug" flag is cleared.
• Characterization test 2D area excludes the flat-dimension spacing (all three flat orientations, non-unit flat spacing) pins it.
• Thread safety: gated both parallel scans with requireStoresInMemory(featureIds).
• Provenance: corrected the stale canonical oracle block (2358.834→2362.434, etc.) to match the tests and the trace; documented the retired Legacy: Small IN100 Test.
• D1: qualified the Kahan claim (compensator resets per TBB chunk; the float64 accumulation is the dominant improvement).

All 10 ComputeFeatureSizes TEST_CASEs pass in-core.

One open item for you: the PR title "Thread Summation Patch" implies a concurrency race, but the changed code is the serial combine_each reduction (swapped Kahan operands, a precision fix) — no race existed. Consider retitling to reflect the precision fix + the 2D-area bug fix.