[jmarquisbq]

Summary

Correct X/Y voxel-face weights and sphericity exponents in Compute Surface Area to Volume & Sphericity. Use Float64 accumulation and metric arithmetic while retaining Float32 outputs, validate selected label arrays during preflight, and process Scanline metrics with bounded transfer buffers. Add analytical regressions and scientific documentation. 14 files changed, +1425 / −139 lines.

Companion: BlueQuartzSoftware/SimplnxOoc#6 — TEST: Verify surface metrics on HDF5 stores supplies actual-OOC regression cases and depends on the shared fixture header added here. This public correction can merge first and builds in standalone in-core simplnx. #1817 must merge before simplnx #1826, which migrates other filters to the shared tuple validator.

1. Correct physical surface metrics

• src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ComputeSurfaceAreaToVolumeDirect.cpp implements resident neighbor traversal.
• src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/ComputeSurfaceAreaToVolumeScanline.cpp implements slice-based bulk I/O.

X-normal faces use dy*dz, Y-normal faces use dx*dz, and Z-normal faces use dx*dy. Both algorithms accumulate in Float64, use cbrt(pi) and 2.0/3.0 for sphericity, and convert only the final values to Float32. Feature counts use usize; the pi-root calculation runs only when sphericity is requested.

The calculation keeps supplied NumCells volumes, internal zero-label interfaces, and omitted image-exterior faces. The face-weight change affects unequal X/Y spacing. Precision and exponent corrections can also change results for equal spacing.

2. Validate selected arrays and geometry counts

• src/simplnx/Utilities/DataArrayUtilities.hpp and .cpp provide ValidateCellTupleCount.
• src/Plugins/SimplnxCore/src/SimplnxCore/Filters/ComputeSurfaceAreaToVolumeFilter.cpp applies the metadata checks before output actions.

The count-only validator reports the selected paths, actual and expected counts, and corrective action. Matching counts remain valid across different parents or tuple shapes. The label ordering must describe the selected image; checking that correspondence remains the user's responsibility.

Temporary concrete-type guards reject StringArray and NeighborList selections at the scalar-int32 inputs without throwing. The parameter-level replacement is tracked in #1833, following #1821, and is not a merge prerequisite. Keep the guards until shared validation provides equivalent protection while preserving intentional generic-array consumers.

3. Bound Scanline transfer scratch

• ComputeSurfaceAreaToVolumeScanline.cpp releases label slices before metric transfers and reuses one result buffer for both outputs.
• ComputeSurfaceAreaToVolumeScanline.hpp documents memory and failure behavior.

Counts and results use at most 512 KiB of combined transfer scratch. For at least two feature tuples, that scratch also uses no more than four bytes per feature. The Float64 area accumulator still scales with feature count; overall working memory remains O(X*Y + F), excluding stores and caches. Late metric-phase read or write errors can leave completed batches written.

4. Verify numerical, storage, and documentation contracts

• src/Plugins/SimplnxCore/test/ComputeSurfaceAreaToVolumeTestUtils.hpp supplies shared analytical fixtures.
• src/Plugins/SimplnxCore/test/ComputeSurfaceAreaToVolumeTest.cpp covers input contracts, algorithm paths, batch boundaries, and late I/O failures.
• src/Plugins/SimplnxCore/docs/ComputeSurfaceAreaToVolumeFilter.md explains the measured surface, nonfinite results, single slices, and numerical compatibility.

Box, connected-comb, and equal-volume-sphere expectations provide independent correctness checks. The legacy Small IN100 archive remains a compatibility control. The figures show correct voxel-face areas and a clearly labeled anisotropic Small IN100 copy with comparable physical views and a shared Viridis scale.

Actual HDF5 tests run through DREAM3DNX with DREAM3DNX_USE_OOC=ON, SIMPLNXOOC_SOURCE_DIR set to the paired checkout, and SIMPLNX_TEST_ALGORITHM_PATH=1. Host wiring is already merged through DREAM3DNX #1121; the private test infrastructure includes merged SimplnxOoc #3. Use matching surface-area branches until the companion changes merge. The broader OOC audit series is not a prerequisite for this verified combination.

All six companion cases check OOC storage and dispatch. The comb, scale, nonpositive-count, and sparse-feature cases additionally assert concrete HDF5 stores and existing backing files; the pair and single-slice cases currently use the base store witness.

Test plan

Validation below covers simplnx 7f2c0f304 with SimplnxOoc a17ea267d on Windows.

• In-core and OOC Release ALL_BUILD targets pass.
• Selected filter/helper tests pass: 14/14 in-core Release and 20/20 OOC Release, run serially.
• In-core Debug test targets build and the same 14/14 tests pass, using command-scoped /bigobj for the existing large test-translation-unit limit.
• New bounded-transfer and later-I/O-failure tests fail the prior full-vector implementation and pass the fix.
• Independent analytical expectations, dispatch witnesses, and the stated actual-HDF5 assertions pass.
• Changed C++ files pass clang-format 19.1.1, comment audit, and whitespace checks.
• Configured clang-tidy 19.1.5 diagnostics are reviewed with case-aware filters and header coverage. No actionable changed-code findings remain. Raw runs retain Catch2/Result-macro diagnostics and an unchanged Result.hpp brace warning; they are not zero-warning passes.
• Current-head CI passes on all configured platforms. See PR checks.

Related Issue

Closes #1773
Closes #1780
Related to #1821
Related to #1833

[imikejackson]

Review summary

Reviewed at commit 58f9f72. All 7 changed files were read.

Verdict: request changes. The numeric fix, the test oracles, and the documentation are sound. The requested changes are a shared tuple-check helper (or a follow-up issue with aligned wording), the <cmath> includes, a more specific -12804 message, and two documentation wording fixes. None of them change results. Details are in the inline comments.

Confirmed correct

• Face weights now match the geometry in both paths: +/-Z faces use dx*dy, +/-Y faces use dx*dz, +/-X faces use dy*dz. develop had X and Y swapped in both Direct and Scanline.
• Sphericity uses exact exponents (cbrt(pi) and 2.0/3.0) instead of 0.333333f and 0.66666f.
• I re-derived the connected-comb oracle by hand. The cell count (138957) and the face counts (7770 / 270214 / 270366) are correct, and the one-cell border keeps every comb face inside the image.
• The interior-pair oracle correctly reuses the same area for the enclosing feature because its image-exterior faces are omitted. The sphericity expected value comes from the equal-volume sphere radius, so it is independent of the implementation formula.
• The nonfinite cases match the code: count 0 gives +inf ratio and 0 sphericity; count -2 gives a negative ratio and NaN sphericity.
• The tuple-count test also covers a label geometry with a different shape but the same cell count (6x5x2 vs 4x3x5), which documents that only the count is checked.
• No memory, lifetime, or thread-safety issues. All changed C++ files pass clang-format -style=file with the pinned clang-format version.

Local test evidence

Build	Result
ci-macos-arm64 in-core, SIMPLNX_TEST_ALGORITHM_PATH=0	9/9 pass, 0 compiler warnings
DREAM3DNX OOC build, SIMPLNX_TEST_ALGORITHM_PATH=1	9/9 pass, 0 compiler warnings

Not run locally: the five HDF5 cases in SimplnxOoc #6. The Test Plan boxes in the PR body are still unchecked and the body says post-rebase validation is running; please update before merge.

[jmarquisbq]

Addressed the review feedback:

• Added and used the shared count-only geometry validator with consistent path/count diagnostics. The other-filter migrations are in #1826.
• Included the actual selected array kind in the FeatureIds error and ordered the two new error codes. Added diagnostic assertions for StringArray and NeighborList inputs.
• Added the explicit math/vector headers and corrected include order. Simplified Direct accumulation, reused the current label, and made the loop comparison consistent.
• Ended the Scanline ratio-buffer lifetime before sphericity allocation, reducing live feature-buffer payload from 20 to 16 bytes per feature when sphericity is enabled.
• Clarified voxelized-sphere sphericity and the exact X/Y spacing conditions for changed results.
• Added the six-face schematic and a verified Small IN100 before/after comparison. The comparison explicitly labels altered spacing, preserves physical aspect, and uses larger panels with a shared 0–1 Viridis scale.
• Reproduced the shared array-selection gap and filed #1821 with failing regression tests and passing compatibility controls.

Validation: full Windows Release builds passed for in-core and OOC. The focused utility/surface suites passed 10/10 in-core, 15/15 OOC, and 10/10 Debug. Debug used /bigobj for a separately reproduced pre-existing object-file limit. The figure calculations were checked independently for all 2,317 populated positive features. Float32 output arrays are retained.

[imikejackson]

Review of PR #1817 at 008acec89

Reviewed every one of the 14 changed files at this commit. Built SimplnxCoreUnitTest and simplnx_test locally with the ci-macos-arm64 preset (Release, in-core scenarios) and ran the affected tests serially: 11/11 pass (the 9 ComputeSurfaceAreaToVolume* tests, ValidateCellTupleCount, IsChildOfAttributeMatrix). clang-format 19.1.1 with the repo .clang-format reports no changes on any of the changed C++ files.

Confirmed correct (independently verified)

• Face-weight fix is right in both algorithms. In Direct, offsets 1/4 are the ±Y neighbors and now add dx*dz; offsets 2/3 are ±X and now add dy*dz; 0/5 (±Z) keep dx*dy. Scanline's y±xPoints neighbors add zxFaceArea and x±1 add yzFaceArea. This matches the tables in #1773 and #1780.
• Exponents 0.333333f / 0.66666f replaced by cbrt and 2.0/3.0 in float64; outputs stay float32.
• Analytical oracles are real oracles (box, single slice, comb with STATIC_REQUIREd face counts, scale invariance, nonpositive counts). They do not re-run the production neighbor scan. Good.
• No cell-sized temporaries added. Direct adds one float64[F]; Scanline keeps three int32[X*Y] slice buffers and uses F-sized caches only. Acceptable for OOC.
• The -12803 path is reachable: ArraySelectionParameter::validatePath only applies AllowedTypes/component-shape rules to IDataArray, so a StringArray or NeighborList at an int32-only parameter passes parameter validation and reaches preflightImpl. I confirmed this in ArraySelectionParameter.cpp.
• Doc images render; the face-area table in ..._VoxelFaceAreas.png is arithmetically correct for the (0.25, 0.50, 0.25) demo spacing. Both SVGs embed their rasters as data URIs, so no dangling references.

Requested changes

1. (Medium, design) The -12803 guard treats a symptom whose cause is in ArraySelectionParameter.
A StringArray or NeighborList can never satisfy a non-empty numeric AllowedTypes, yet validatePath lets both through. Fixing it there (return k_Validate_AllowedType_Error when the object is an IArray but not an IDataArray and m_AllowedTypes is non-empty) protects every filter at once, and the rejects non-DataArray selections test here would then assert the parameter error instead of a filter-private code. #1826 is about to copy this per-filter pattern into three more filters, so please decide this before both PRs land. If you prefer to keep the filter guard as defence in depth, say so in a comment, but the parameter fix should still happen (here or in a tiny preceding PR).

2. (Low) Direct/Scanline drift. Direct computes thirdRootPi unconditionally (line 141) while Scanline computes it only inside the sphericity block; Direct casts numFeatures from usize to int32 and then back to usize in both loops. Make numFeatures a usize in both algorithms and move thirdRootPi into the sphericity block in Direct. Scanline's auto& featureIdsStore (line 33) should be const auto&.

3. (Low, tests) CheckInteriorPairSurfaceMetrics takes dimensions but REQUIREs it to be exactly {5,5,5} or {5,5,1}. The parameter is a boolean in disguise. Either take bool singleSlice or honour arbitrary dimensions (the oracle already works for any box that leaves the pair interior).

4. (Low, docs) Fig. 2 alt text is three sentences long. Alt text should be a short description; move the caption into body text under the image. Also, the sentence "Interpret these values as limitations of the supplied volume or measured surface" will not be clear to non-native readers. Suggest: "These values mean the input cell count or the measured surface is not valid for that feature. Check the Number of Cells array and the feature labels."

5. (Low, process) PR description is stale. It says "7 files changed, +672 / -40"; the PR is 14 files, +871 / −53. The Test Plan records only Windows runs and CI is unchecked. Please refresh before merge.

Notes, not blocking

• The pre-existing SimplnxCore::ComputeSurfaceAreaToVolume archive test still passes, but only because CompareDataArrays uses an absolute EPSILON = 1e-4 and the exponent correction shifts Small IN100 sphericity by roughly 3e-5 to 6e-5. That exemplar was produced by the old code. The new analytical tests are the real oracle for this PR; consider adding a comment in the archive test saying so, otherwise a future reader may believe the exemplar validates the exponents.
• ValidateCellTupleCount compares counts only, by design. The docs say the label array's "cell ordering" must match the image, which the filter cannot check. That is fine, but the doc should say it is the user's responsibility.
• Issue #1773 says the OOC audit series (#1764) is related. This PR is independent of it, which is good.

Verdict

REQUEST CHANGES for items 1 and 5; 2 to 4 are small and can ride along.

[jmarquisbq]

Addressed the follow-up review and the additional adversarial-review findings at 7f2c0f304:

• Kept the local concrete-array guards and documented the parameter-level replacement in BUG: Enforce array-kind contracts in ArraySelectionParameter #1833. That separate change remains nonblocking; generic StringArray/NeighborList compatibility is preserved.
• Used usize feature counts, made the Scanline label store const, and limited the pi-root calculation to requested sphericity.
• Replaced the fixture's two-valued dimensions argument with bool singleSlice and updated the paired SimplnxOoc callers.
• Shortened figure alt text, moved the caption into the body, clarified nonfinite results and label-ordering responsibility, and explained the legacy exemplar's tolerance limits.
• Released label slices before metric processing and bounded count/result scratch to 512 KiB. Float64 accumulation and Float32 outputs remain unchanged.
• Added analytical batch-boundary cases, actual-HDF5 coverage, and late read/write failure probes. The transfer/failure tests failed the previous implementation before passing the fix.
• Fixed direct includes, naming, initialization, and related coding-standard findings. The updated description records current dependencies, scope, and verification.

Windows Release builds pass in both configurations. Selected tests pass 14/14 in-core Release, 20/20 OOC Release, and 14/14 in-core Debug with SimplnxOoc a17ea267d. Independent follow-up reviews found no actionable issues. Formatting passes. Corrected clang-tidy runs have no actionable changed-code findings; raw framework-macro diagnostics and the inherited Result.hpp warning remain explicitly classified. The earlier description's clean clang-tidy claim came from a filename-filter mismatch and has been corrected.

The six addressed open inline threads are resolved. The shared parameter redesign remains in #1833, and #1817 must merge before #1826. Current-head CI is still pending; no completed current-head CI result is claimed here.