[imikejackson]

Summary

These filters read Cell-level DataArrays of an Image Geometry (or another grid geometry) by cell index, using indices computed from the geometry's dimensions. Preflight never checked that the DataArray has one tuple per cell. A shorter DataArray was read past its end during execute, which crashed or produced garbage. A longer one was silently truncated. Preflight now compares each such input's tuple count with the geometry's number of cells.

These filters only read the inputs, so the DataArray may be in any Attribute Matrix. Only its tuple count must match. Each filter's test checks both directions: a short DataArray is rejected, and a correctly sized DataArray in another Attribute Matrix is still accepted.

The checks use ValidateCellTupleCount in DataArrayUtilities, which #1817 added, plus one new overload that takes a DataStructure and several DataPaths. It reads tuple counts through IArray, so a StringArray or NeighborList cannot throw std::bad_cast. Each filter has its own commit, a regression test that fails on develop, and one sentence in its documentation. No test data archive is added or changed.

Behavior changes (new preflight errors)

SimplnxCore

Filter	Inputs checked	Code
AddBadData	Boundary Euclidean Distances	−76238
AlignSectionsFeatureCentroid	Mask Array (replaces a check that compared the mask with itself)	−68071
ComputeBoundingBoxStats	Attribute Array to Compute Statistics	−59204
ComputeEuclideanDistMap	Cell Feature Ids	−12805
ComputeFeatureCentroids	Cell Feature Ids	−12701
ComputeFeatureNeighbors	Cell Feature Ids	−24501
ComputeFeatureSizes	Cell Feature Ids	−74771
ComputeMomentInvariants2D	Cell Feature Ids	−9511
ComputeShapes	Cell Feature Ids	−12806
ComputeSurfaceFeatures	Cell Feature Ids	−1099
ErodeDilateMask	Mask Array	−14702
ExtractFeatureBoundaries2D	Feature Ids	−1019
FlyingEdges3D	Data Array to Contour	−10510
IdentifySample	Mask Array	−45463
InitializeImageGeomCellData	Cell Arrays; the old tuple-rank check (−5560) is removed, so any tuple shape with one tuple per cell is accepted	−3557
M3CSurfaceMeshing	Cell Feature Ids, Cell Attribute Arrays to Transfer	−90201
NearestPointFuseRegularGrids	Sampling and Reference Cell Attribute Matrices	−5565
PartitionGeometry	Input Geometry Cell Attribute Matrix (existing partitioning scheme mode)	−3045
ReplaceElementAttributesWithNeighborValues	Input Comparison Array	−15001
ScalarSegmentFeatures	Scalar Array to Segment, Cell Mask Array (when used)	−654
WriteAbaqusHexahedron	Cell Feature Ids	−1124
WriteAvizoRectilinearCoordinate	Cell Feature Ids	−5834
WriteAvizoUniformCoordinate	Cell Feature Ids	−5835
WriteLosAlamosFFT	Cell Feature Ids, Cell Euler Angles, Cell Phases	−73461
WriteSPParksSites	Cell Feature Ids	−77453

OrientationAnalysis

Filter	Inputs checked	Code
AlignSectionsMisorientation	Cell Quaternions, Cell Phases, Cell Mask Array (when used)	−68072
AlignSectionsMutualInformation	Cell Quaternions, Cell Phases, Cell Mask Array (when used)	−3543
BadDataNeighborOrientationCheck	Cell Quaternions, Cell Phases, Cell Mask Array	−6810
ComputeFeatureReferenceCAxisMisorientations	Cell Feature Ids, Cell Phases, Cell Quaternions	−9805
ComputeKernelAvgMisorientations	Cell Feature Ids, Cell Phases, Cell Quaternions	−67205
NeighborOrientationCorrelation	Confidence Index, Cell Phases, Cell Quaternions	−580098
WriteINLFile	Cell Feature Ids, Cell Phases, Cell Euler Angles	−78431

Included from #1826 (Jessica Marquis)

#1826 is closed and its five commits are included here with their authorship. They move three filters' existing tuple-count checks onto ValidateCellTupleCount and add type checks for selections the parameters do not yet restrict (#1833):

• RequireMinNumNeighbors: Feature Ids tuple count −59571 (was −55571); type checks for Feature Ids −59574, Number of Neighbors −59575, Feature Phases −59576. The codes moved to −5957x so they do not collide with BUG: Require Feature Ids in the geometry's Cell Attribute Matrix for filters that modify it #1829, which uses −55573 to −55575 in this filter.
• PartitionGeometry: cell checks −3009 and −3010 use the shared helper; node geometries report a separate vertex tuple-count error (−3043); Vertex Mask type check −3044.
• WriteOnScaleTableFile: cell check uses the shared helper; Feature Ids type check −12042.

Not in this PR

• ComputeSurfaceAreaToVolume already has this check on develop (BUG: Correct surface-area and sphericity calculations #1817).

• ComputeFeatureRect has no geometry parameter. It takes its dimensions from the Feature Ids tuple shape and assumes that shape has three dimensions. That needs a different fix.

• Filters that modify Feature Ids or its sibling arrays use the stricter Cell Attribute Matrix rule in BUG: Require Feature Ids in the geometry's Cell Attribute Matrix for filters that modify it #1829.

• Triangle, Edge and Vertex geometry filters with the same kind of gap will be handled separately.

Other changes

• Documentation errors found while editing, each checked against the code: ComputeFeatureNeighbors (Shared Surface Area is an area that uses the cell spacing), AlignSectionsFeatureCentroid (removed a section about a parameter that does not exist), AddBadData (at least one noise mode must be enabled).
• ComputeFeatureSizes: four invalid-dimension tests were reaching the new tuple-count error first; their fixtures now match the geometry again.

Test plan

• Each new regression test passes. Each one fails on develop because preflight accepts a DataArray whose tuple count differs from the cell count.
• The full test suite passes (3288 of 3289 locally on macOS; the test that does not run needs Python, which the local build does not include).
• Preflighting every simplnx example pipeline gives the same results as before this change.
• CI passes on all supported platforms.

[jmarquisbq]

Reviewed all 108 changed files at commit 020358f0e80e103140096a475dae97cad897aae4.

Findings

• Critical — Windows test-build failure. WriteINLFileTest.cpp:257 passes a StringLiteral directly to fs::path. MSVC rejects this with C2440, blocking OrientationAnalysisUnitTest. Use the existing portable convention: fs::path(unit_test::k_BinaryTestOutputDir.view()).

• Should-fix — qualify the external-array documentation. ReplaceElementAttributesWithNeighborValuesFilter.md:83 says the comparison array can be in any Attribute Matrix. With Loop Until Gone enabled, execution updates only the geometry's Cell Data arrays. For a two-cell image, an external comparison array [0,1], and a less-than threshold of 0.5, the comparison remains unchanged. The loop repeats until cancellation. This runtime limitation predates this PR. The new documentation needs this condition:

  When Loop Until Gone is enabled, select a comparison array in the geometry's Cell Data Attribute Matrix. Otherwise, another Attribute Matrix is allowed.

No other material introduced correctness, CPU, or memory findings emerged. The shared helper checks metadata without reading array values. Synthetic fixtures independently test the cell-count contract and valid alternate parents and shapes.

Validation and limits

• All 73 changed C/C++ files passed clang-format 19.1.1 using the repository-root style. The diff whitespace check passed.
• Scoped clang-tidy checks of the added helper lines passed. The changed-line comment audit found no diagnostics. These are scoped results, not a full-project static-analysis pass.
• 248 focused SimplnxCore tests passed after four stale plugin binaries were temporarily quarantined. The original test run failed during plugin loading. All four binaries were restored, with their SHA-256 hashes verified.
• The Windows Release aggregate build failed at the reported compile error. It also reported an additional, unattributed NXVtkLib duplicate-symbol link error.
• OrientationAnalysis runtime tests remain blocked by the compile error. No disk-backed execution was run for this review. GitHub platform builds were still in progress at the final review check.

Verdict: REQUEST CHANGES.

[imikejackson]

Addressing @jmarquisbq's review (fixed in cbb9f1d):

1. Windows test-build failure — fixed. WriteINLFileTest.cpp now builds the output path with fs::path(unit_test::k_BinaryTestOutputDir.view()). I found no other place that passes a StringLiteral directly to fs::path. The WriteINLFile tests pass locally (4/4).
2. External comparison-array documentation — fixed. ReplaceElementAttributesWithNeighborValuesFilter.md now says: "When Loop Until Gone is enabled, select a comparison array in the geometry's Cell Data Attribute Matrix. Otherwise, another Attribute Matrix is allowed." The runtime loop with an external comparison array existed before this PR and is out of scope here.

[jmarquisbq]

Reviewed simplnx #1840 at 99f454c9be11cf58a5d2b366fa66b82751cf89dc. All 107 changed files are accounted for, with unchanged content verified against the previous full review. Both earlier findings are fixed: the INL test uses .view() for the filesystem path, and the Loop Until Gone documentation limits external comparison arrays.

Finding

• Should-fix — verify the split-matrix repair contract. The matching case in NeighborOrientationCorrelationTest.cpp:1175 puts Confidence Index, Phases, and Quaternions in separate Attribute Matrices and checks only preflight. Execution copies replacements only within the Confidence Index parent matrix. Thus, confidence can be repaired while an external quaternion remains unchanged. This existing execution limitation conflicts with the newly documented repair guarantee in NeighborOrientationCorrelationFilter.md:29.

  Clarify the copy boundary and add an execution assertion for the supported layout. Suggested documentation replacement:

  Each selected input must have one tuple per geometry cell. Replacement copies affect arrays in the Attribute Matrix that contains Confidence Index, except ignored arrays. Keep Cell Phases and Cell Quaternions in that matrix when they must also be repaired.

This finding is based on source inspection. The added split-matrix test checks only preflight and does not establish the documented execution behavior.

Confirmed correct

The shared validation uses tuple-count metadata without reading array values or adding cell-sized buffers. Synthetic fixtures independently test the cell-count contract. No additional correctness, efficiency, ownership, design, or defensive-handling findings were found.

Validation and limits

• All 73 changed C++ files passed clang-format 19.1.1 with the repository-root style. The diff whitespace check passed.
• Scoped clang-tidy 19.1.5 passed for changed lines in the shared helper, Partition Geometry, and INL test. Changed-line comment checks passed. These results do not establish a clean full-project static-analysis or full-file comment-audit run.
• Exact-head Windows, GCC, Clang, and macOS CI passed with 3,382 tests each.
• No new local build or disk-backed OOC run was performed.

Verdict: REQUEST CHANGES — correct the repair contract and its positive test.

[imikejackson]

Addressing @jmarquisbq's review at 99f454c:

1. Split-matrix repair contract: fixed in fe9091e. The tuple-count test now runs the filter on a 2×2×2 image with one bad corner cell, in two layouts:

   • All three inputs in one external Attribute Matrix: confidence, phase and quaternion are all repaired, and an array in Attribute Arrays to Ignore stays unchanged.
   • Inputs in separate matrices: confidence is repaired, and the external phase and quaternion stay unchanged. This is the documented limit.

   Before the change, the new execution assertions failed on the old split-only fixture: phases[0] was 0 instead of 1. Execute behavior is unchanged.

Rebase: the branch is rebased onto the current develop, which now includes #1829. RequireMinNumNeighbors conflicted with the commits from #1826. Both sides are kept:

• BUG: Require Feature Ids in the geometry's Cell Attribute Matrix for filters that modify it #1829's checks run first (−55575, −55573, −55574).
• The −5957x checks follow. The Feature Ids type check now runs before the Int32Array cast.

#1829 requires Feature Ids in the geometry's Cell Attribute Matrix, so two fixtures had to move. 4ffce70 moves them to allowed locations, and the tests still check the same tuple-count and error-order behavior.

The affected unit tests pass locally (358/358, macOS arm64 Release).

[jmarquisbq]

The only other thing the review found at this point was the V&V updates which we agreed would be handled separately all at once before releases so I am approving this.