VV: Erode Dilate Bad Data

[mmarineBlueQuartz]

• Fix a bug where X / Y / Z dimensions could not be turned off despite having a parameter to do so.
• Add preflight errors if all X / Y / Z dimensions are disabled.
• Add preflight error if the ImageGeom is missing dimensions.
• Additional unit testing for code coverage.

Naming Conventions

Naming of variables should descriptive where needed. Loop Control Variables can use i if warranted. Most of these conventions are enforced through the clang-tidy and clang-format configuration files. See the file simplnx/docs/Code_Style_Guide.md for a more in depth explanation.

Filter Checklist

The help file simplnx/docs/Porting_Filters.md has documentation to help you port or write new filters. At the top is a nice checklist of items that should be noted when porting a filter.

Unit Testing

The idea of unit testing is to test the filter for proper execution and error handling. How many variations on a unit test each filter needs is entirely dependent on what the filter is doing. Generally, the variations can fall into a few categories:

• 1 Unit test to test output from the filter against known exemplar set of data
• 1 Unit test to test invalid input code paths that are specific to a filter. Don't test that a DataPath does not exist since that test is already performed as part of the SelectDataArrayAction.

Code Cleanup

• No commented out code (rare exceptions to this is allowed..)
• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)
• Added test (or behavior not changed)
• Updated API documentation (or API not changed)
• Added license to new files (if any)
• Added example pipelines that use the filter
• Classes and methods are properly documented

[imikejackson]

Full V&V Review — ErodeDilateBadData

Review methodology: every claim in vv/ErodeDilateBadDataFilter.md and vv/deviations/ErodeDilateBadDataFilter.md was independently verified; the branch was built and tested locally; an independent re-implementation of the algorithm (all 3 variants: develop, this PR, and correct direction gating) was written and swept over all 28 parameter combinations; and a full A/B against DREAM3D 6.5.171 PipelineRunner vs this branch's nxrunner was executed on the PR's exact 4×4×2 fixture (legacy-format input authored with the standard A/B tooling, 56 pipeline runs total).

Blocking: the direction fix is incorrect and regresses default behavior

adjustValidNeighbors() bitwise-ANDs the face-index values returned by initializeFaceNeighborInternalIdx() ({0,1,2,3,4,5} = [-Z,-Y,-X,+X,+Y,+Z]) with the direction booleans. Since 2&1=0, 3&1=1, 4&1=0, 5&1=1, with all three directions enabled (the default) the iterated face list becomes {0,1,0,1,0,1} — the algorithm only ever visits the -Z and -Y neighbors (each 3×), and never -X/+X/+Y/+Z. Every other flag combination also reduces to the face set {-Z, -Y}. Two independent problems:

1. x &= bool on an index is semantically meaningless — it can only map indices to 0 or 1 (which are themselves valid face indices, so disabled faces silently become extra -Z/-Y visits, not skipped faces).
2. Even the intended pairing is wrong: standardNeighbors[3] (+X) is masked with zDir and [5] (+Z) with xDir. The correct axis order for indices 0–5 is z, y, x, x, y, z.

A/B evidence (all runs on this branch's build):

Comparison	Result
6.5.171 PipelineRunner vs this branch nxrunner, 28 combos (Erode/Dilate × 1,2 iters × 7 direction combos)	0/28 match
6.5.171 output vs independent simulation of correct gating (±X←XDirOn, ±Y←YDirOn, ±Z←ZDirOn, per legacy ErodeDilateBadData.cpp)	28/28 match
This branch's output vs independent simulation of the &= masking bug	28/28 match
develop (pre-PR) vs 6.5.171 for the all-directions-on case	match

So the PR breaks the default all-on case that develop computed correctly (develop's actual bug was only that the flags were ignored). Concrete example, Dilate/1 iteration/XYZ on the fixture: 6.5.171 newly zeroes the twelve good voxels {1, 4, 6, 9, 11, 12, 15, 16, 26, 27, 29, 30} (e.g. voxel 1, the +X neighbor of bad voxel 0, must become bad); this branch zeroes only {6, 9, 15, 27} — exactly the -Z/-Y-only result, and exactly what the test arrays expect.

• Replace adjustValidNeighbors with real gating — e.g. build the enabled-face list once before the loops: keep face index i only if {zDir,yDir,xDir,xDir,yDir,zDir}[i] is true — and verify against 6.5.171 for all 7 direction combinations, not just all-on.

Blocking: the test oracle is circular

The V&V report classifies the oracle as Class 1 (Analytical), hand-traced. That claim does not hold: the hardcoded exemplarData/exemplarFeatures arrays match this branch's (broken) output in 28/28 cases and match 6.5.171 in 0/28. Whatever their provenance, they encode the implementation under test, which is the circular-oracle situation the V&V policy explicitly forbids. The report's own observation that all 7 direction combinations have byte-identical expected arrays is not a fixture property — it is a symptom of the bug (all combos degenerate to the same {-Z,-Y} face set, and the visit-multiplicity differences can never change the erode vote winner). Under correct gating this same fixture does discriminate the direction combinations.

• Regenerate all expected arrays from DREAM3D 6.5.171 runs (or a genuinely independent hand-trace) — the fixture itself is good and produces distinct output per direction combo once gating works. I have the full set of 28 legacy-generated outputs from this review and can supply them.
• Collapse the 28 copy-pasted CheckData* functions (all identical bodies per op/iteration today) into one table-driven lookup (operation, iterations, directions) → expected arrays — after the fix the arrays will genuinely differ, and the nested if/else dispatch becomes a map lookup.
• Update the V&V report: the Oracle section (Class 1 claim + "hand-derived" statement), the direction-invariance discussion, and the code-path table (paths through -X/+X/+Y/+Z neighbor visits are currently never executed in any test, so "8 of 9 exercised" is not accurate as written).

Tests: gaps and correctness

• (Dilate) No Dimensions never reaches the zero-dimension check: it also disables all directions, so preflight fails at -14601 before -14602 (the report admits this). Enable at least one direction and assert the specific error code (-14602); also assert -14601 explicitly in (Dilate) No Direction — both tests currently accept any preflight failure.
• CreateTestData, RunFilter, and all CheckData* helpers sit outside the anonymous namespace (after } // namespace, ErodeDilateBadDataTest.cpp:40). ComputeFeatureRectTest.cpp already defines a CreateTestData() — it is only saved by its anonymous namespace. Move all file-local helpers into the anonymous namespace to remove the ODR hazard in the unity test binary.
• IgnoredDataArrayPaths is never exercised — add a case with an ignored array and assert it is untouched.
• The retired exemplar test covered multiple array types/components; the new fixture only exercises single-component int32. Add at least one multi-component array (e.g. float32 ×3) to the fixture so copyTuple stride handling stays covered. (Retiring the archive itself is fine per policy; note 6_6_erode_dilate_test.tar.gz must stay in test/CMakeLists.txt — ErodeDilateMaskTest and ErodeDilateCoordinationNumberTest still use it.)
• CheckDataErode takes Int32Array& while CheckDataDilate takes const Int32Array& — make both const.

Preflight / filter code

• The dimension check errors only when all three dims are zero (&&). A geometry with e.g. {4,4,0} passes preflight and silently no-ops. Use || (any zero dimension) unless there is a reason not to.
• int32 k_NoDirections_Error / k_NoGeometryDimensions are mutable globals with inconsistent naming — make them constexpr int32 and follow k_CamelCase (k_NoDirectionsError, k_NoGeometryDimensionsError).
• auto& imageGeom in preflight should be const auto&.
• Typo in the algorithm comment: "acccount". Missing trailing newline at EOF in both ErodeDilateBadData.cpp and ErodeDilateBadDataTest.cpp.

V&V document corrections (beyond the oracle section)

• "Verified commit 3cd0f6cbd (branch vv/ErodeDialateBadData)" — that commit does not exist in this repository and the branch name is misspelled; the At-a-glance item "commit the new test case (currently uncommitted on this branch)" is stale (it is committed in this PR). Update provenance to the actual PR head.
• The report/deviations claim legacy comparison "was not possible" — the 6.5.171 legacy source (Source/Plugins/Processing/ProcessingFilters/ErodeDilateBadData.cpp) and the 6.5.171 PipelineRunner are both part of the standard V&V toolchain and were used for this review. The deviations file also references a Windows path from a different machine (C:\Users\holym\...) — remove. Perform and record the legacy comparison as real Deviation entries (the 28-run matrix above can seed it).
• The "chosen randomly" tie-break question can be closed now: legacy uses strict current > most — first-processed neighbor wins, fully deterministic, same face order as NX. docs/ErodeDilateBadDataFilter.md should be corrected rather than carrying the legacy wording forward.

Algorithm quality (pre-existing, but in scope for a V&V pass)

• ErodeDilateBadDataTransferDataImpl stores std::vector<int64> m_Neighbors by value — every parallel task copies an 8-byte-per-voxel vector, per array, per iteration (~1 GB per copy on a 500³ volume). Hold a const std::vector<int64>& (lifetime outlives taskRunner.wait()) or a shared immutable buffer. m_FilterAlg is also an unused member.
• GenerateDataArrayList and MessageHelper are reconstructed inside the iteration loop — hoist above it.
• operator() never checks getCancel() — a long multi-iteration run on a large volume cannot be cancelled. Add a cancel check per iteration (or per z-slice).

Verified-correct claims (for the record)

The following report claims were independently confirmed: 5/5 test cases pass with exactly 1883 assertions on this branch; legacy UUID 3adfe077-… → ErodeDilateBadDataFilter mapping and both simpl_conversion fixtures; legacy Direction: 0 = Dilate = NX operation_index 0; the SIMPL backwards-compatibility test content; the absence of any cancel path; and the report's own admissions about the -14601/-14602 test overlap and the identical per-direction arrays. Parameters were not changed, so no parametersVersion bump is required.

[imikejackson]

See comments for feedback