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.

• 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.
• 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.
• 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

[imikejackson]

PR 1687 Re-Review — VV: Erode Dilate Bad Data

Re-review of head 42934ffcd against the 2026-07-24 review. Branch built and all 7 tests run locally; every prior checkbox re-verified against the tree.

Both blocking items are resolved — verified independently

Direction gating is now correct. adjustValidNeighbors masks the per-voxel isValidFaceNeighbor array with the right axis pairing instead of bitwise-ANDing face indices.

I re-derived the oracle from scratch rather than trusting the fix: read the legacy source (Source/Plugins/Processing/ProcessingFilters/ErodeDilateBadData.cpp, 6.5.172 checkout), wrote a standalone simulator of it, ran all 28 combinations (2 ops × 7 direction combos × 2 iteration counts) on this PR's 4×4×2 fixture, and diffed against the committed k_Exemplar* constants:

Check	Result
Committed expected arrays vs. independent simulation of legacy 6.5.172 semantics	28/28 exact (FeatureIds and Misc)
Committed expected arrays vs. simulation of this PR's semantics	28/28 exact
Prior review's 28/28 legacy-vs-&=-bug mismatch	no longer reproduces

The oracle is no longer circular. The arrays now discriminate direction combinations and agree with legacy element-wise. Whatever their stated provenance, they are independently correct.

Tests: 7/7 pass, 2039 assertions (55 + 939 + 939 + 67 + 9 + 3 + 27). Note: the run initially failed on a stale sibling DREAM3D_Plugins/SimplnxReview checkout claiming UUID 5e18a9e2-…, which has since moved to OrientationAnalysis — environmental, unrelated to this PR.

Bugs

• Ignored Path test never executes the filter — it asserts nothing
  File: ErodeDilateBadDataTest.cpp (SimplnxCore::ErodeDilateBadDataFilter Ignored Path)
  The test calls filter.preflight(...) and then CheckPathIgnored, which compares the DataStructure against a freshly built CreateTestData(). Since the filter never ran, nothing could have changed, so the 64 REQUIREs pass vacuously.
  Proven empirically: replacing MultiArraySelectionParameter::ValueType{ignoredPath} with {} (an empty ignore list) and rebuilding — the test still passes. It cannot distinguish "ignored" from "not ignored".
  Fix: add auto executeResult = filter.execute(dataStructure, args); SIMPLNX_RESULT_REQUIRE_VALID(executeResult.result) before CheckPathIgnored. Consider also asserting that FeatureIds did change, so the test proves the filter actually did work.
  Knock-on: the V&V report's test-inventory row — "Confirms an array listed in IgnoredDataArrayPaths (Misc) is left untouched. Passes." — is not true as written.

Memory / Lifetime

No issues found. m_Neighbors is now const std::vector<int64>& (lifetime outlives taskRunner.wait()), and the dead m_FilterAlg member is gone. Both prior findings resolved.

CPU / Algorithm Efficiency

• constexpr was dropped from faceNeighborInternalIdx (regression vs. develop)
  File: Algorithms/ErodeDilateBadData.cpp (operator())
  develop had constexpr std::array<FaceNeighborType, k_NumFaceNeighbors> faceNeighborInternalIdx = initializeFaceNeighborInternalIdx();. This PR removed constexpr — presumably needed by the earlier &=-mutation approach, which is gone. Nothing mutates it now; restore constexpr.

GenerateDataArrayList and MessageHelper are now hoisted above the iteration loop — prior finding resolved.

Naming Consistency

• k_NoGeometryDimensions should be k_NoGeometryDimensionsError
  File: ErodeDilateBadDataFilter.cpp (anonymous namespace)
  Both are constexpr int32 now (prior finding resolved), but the sibling is k_NoDirectionsError. Match it.

• uint64 operation / int32 operation should be ChoicesParameter::ValueType
  File: ErodeDilateBadDataTest.cpp ((Erode) Expanded, (Dilate) Expanded, Ignored Path, No Direction)
  These are immediately fed to std::make_any<ChoicesParameter::ValueType>(operation). It compiles by implicit conversion, but declaring the parameter's own type is clearer and matches k_Dilate/k_Erode above.

Const-Correctness

No issues found. const auto& imageGeom in preflight and const DataStructure& on both check helpers — both prior findings resolved.

Readability

• CheckDilateOutput and CheckErodeOutput are two copies of the same 50-line if/else chain
  File: ErodeDilateBadDataTest.cpp
  Collapsing 28 near-identical functions into 2 is a big improvement over the prior revision, but these two differ only in which constants they name. A single table keyed on (operation, directions, iterations) removes the duplication and the REQUIRE(false) fallthrough entirely.

• REQUIRE(false) on an unhandled direction combination
  File: ErodeDilateBadDataTest.cpp (CheckDilateOutput, CheckErodeOutput)
  Produces REQUIRE( false ) with no explanation. Use FAIL("unhandled direction combination").

• All-directions-off is skipped by a bare return after the fixture is built
  Files: ErodeDilateBadDataTest.cpp ((Erode) Expanded, (Dilate) Expanded)
  CreateTestData() runs and is thrown away for the invalid combination. Move the guard above it, and add a SUCCEED("at least one direction is required") so the skip is visible in the Catch2 output rather than silent.

Formatting: clang-format is not installed on the review machine, so the read-only format gate could not be run — flagging that rather than claiming a pass. By inspection all three files are tab-free, have no trailing whitespace, are ≤199 columns, and end with a newline (prior EOF finding resolved). The "acccount" typo is gone.

Robustness / Defensive

• CheckPathIgnored dereferences raw getDataAs<Int32Array> results without a null check
  File: ErodeDilateBadDataTest.cpp (CheckPathIgnored)
  A typo in ignoredPath yields a segfault instead of a test failure. Project convention is REQUIRE_NOTHROW(ds.getDataRefAs<Int32Array>(path)) followed by getDataRefAs.

• Missing taskRunner.wait() after the FeatureIds transfer (Pre-existing bug surfaced by this PR.)
  File: Algorithms/ErodeDilateBadData.cpp (operator())
  The final taskRunner.execute(...) is correct only because setParallelizationEnabled(false) was called one line earlier. On the next iteration the runner is re-created, so nothing leaks today — but a trailing wait() makes the invariant local instead of implied.

Tests

• k_TupleShape{4, 4, 2} is used as both the AttributeMatrix tuple shape and the geometry dimensions
  File: ErodeDilateBadDataTest.cpp (CreateTestData)
  geom->setDimensions(SizeVec3{4, 4, 2}) is X=4, Y=4, Z=2, but DataArray/AttributeMatrix tuple shape is slowest-to-fastest ZYX (cf. RotateSampleRefFrameFilter.cpp:249, ReadNIfTIFileFilter.cpp:273). The correct tuple shape here is {2, 4, 4}. Harmless today only because both spell 32 tuples — use two separate constants.

• 6 of 7 TEST_CASEs omit UnitTest::CheckArraysInheritTupleDims(dataStructure)
  File: ErodeDilateBadDataTest.cpp
  Only the archive-based (Erode) test calls it. Adding it to the Expanded cases would also have caught the tuple-shape item above.

Prior test findings now resolved: all file-local helpers are inside the anonymous namespace (the ComputeFeatureRectTest.cpp::CreateTestData ODR hazard is gone); No Direction and No Dimensions assert -14601/-14602 explicitly and No Dimensions now enables all directions so it actually reaches the dimension check; and restoring the 6_6_erode_dilate_test.tar.gz exemplar test brings back the multi-type/multi-component copyTuple stride coverage (EulerAngles float32×3, Mask uint8, Phases int32) that the inline fixture alone does not provide.

Documentation

• Verified commit 4437eacda does not exist in this repository (flagged last round, unchanged)
  File: vv/ErodeDilateBadDataFilter.md (header table)
  git cat-file -t 4437eacda → "Not a valid object name". The commit whose message is "Fixing ErodeDilateBadData" is 56b30c923. The branch is also still spelled vv/ErodeDialateBadData; the real name is vv/ErodeDilateBadData. Cite the PR head (42934ffcd) instead.

• Assertion count is stated as 2033; the actual count is 2039
  File: vv/ErodeDilateBadDataFilter.md (Summary)
  Measured per-test: 55 / 939 / 939 / 67 / 9 / 3 / 27.

• Internal contradiction about exemplar archives
  File: vv/ErodeDilateBadDataFilter.md (At-a-glance → Test fixtures)
  "Inline CreateTestData() — no exemplar archive for the automated tests" contradicts the test-inventory row describing (Erode) as "Exemplar-archive-based". Qualify it as "no archive for the Expanded sweep".

• Drop the "hand-traced" provenance claim for the 28 expected arrays
  Files: vv/ErodeDilateBadDataFilter.md (Oracle), vv/deviations/ErodeDilateBadDataFilter.md (B1 Verification)
  1,792 hand-traced values is not a credible claim, and it is not needed: the arrays match genuine 6.5.171 output, which is a stronger oracle. State plainly that they were regenerated from the legacy binary and verified element-wise — Class 2, corroborated. The "Class 1 in form, Class 2 in substance" hedging invites exactly the circularity question it is trying to answer.

• docs/ErodeDilateBadDataFilter.md still says erode ties are "chosen randomly"
  File: src/Plugins/SimplnxCore/docs/ErodeDilateBadDataFilter.md (line 27)
  The deviations doc correctly identifies this as inaccurate legacy language, but the user-facing doc was never corrected and is not in this PR's file list. Replace with the actual behavior (first-processed neighbor in [-Z,-Y,-X,+X,+Y,+Z] scan order wins; fully deterministic). While there, document the two new preflight errors.

Confirmed Correct (no action needed)

• Direction masking of the Erode featureCount reset loop. This PR reuses the direction-masked isValidFaceNeighbor in the reset loop; legacy resets over boundary-valid neighbors and ignores the direction flags there. Simulating both variants across all 28 combinations gives identical output, and it is provably equivalent in general — the reset set is a superset of the increment set in both variants, so featureCount returns to all-zeros after every bad voxel either way. Not a deviation.
• Deferring the FeatureIds transfer to a second pass. Legacy interleaves FeatureIds with the other arrays in one pass and mutates m_FeatureIds as it goes. Equivalent here: erode only maps 0→>0 and dilate only >0→0, neighbors[] always points at a voxel whose relevant polarity is preserved by the operation, and each index is written at most once per pass — so neither transfer predicate can ever observe a changed value.
• neighbors is intentionally not reset between iterations — matches legacy, and the deviations doc records the investigation that established this.
• MessageHelper shared across parallel tasks. Each task gets its own ThrottledMessenger (independent timing state) over a shared std::shared_ptr<Messenger>; trySendMessage is the documented cross-thread path. Safe.
• Cancel check placement (per Z-slice, inside the iteration loop) — real early-exit, negligible cost, ahead of legacy which has none.
• Preflight ordering — -14601 is checked before the geometry lookup, -14602 after; both codes asserted by name in tests.
• FromSIMPLJson and parametersVersion — parameters unchanged, no version bump required; both 6.4 and 6.5 fixtures still convert.

---

The two blocking issues from the last round are genuinely fixed, and the fix has been verified against legacy rather than against this PR's own tests. The one remaining substantive defect is the vacuous Ignored Path test; the rest are nits and V&V-document accuracy.