VV: Require Minimum Number of Neighbors fully V&V'ed

[JDuffeyBQ]

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]

V&V PR Review — RequireMinNumNeighborsFilter

Reviewed at 5f1c7a659. Scope: verification of every claim in the two V&V documents, adversarial review, CPU/memory/OOC review, and an independent re-run of the A/B against 6.5.171 using our own methodology rather than accepting the one in the report.

What holds up

• The "no deviations" conclusion is correct — and I confirmed it with a harder test than the PR ran. I built a legacy-native 6×6×6 fixture that deliberately probes what the 4×1×1 oracle cannot: an order-dependent 1-vs-1 neighbour vote tie, a rejected voxel fully enclosed by rejected voxels (forces a 2nd coarsening iteration — both runners logged 31 → 1 → 0 voxels to update), a 3-component float32 cell array through copyTuple(), an ignored array, and a retained feature owning zero voxels so the two phase modes yield different feature-array lengths from identical cell output. I compared 6.5.171, NX, and an independent NumPy reference implementation of the documented algorithm. All three agree on all six arrays in both modes.
• initializeFaceNeighborOffsets/computeValidFaceNeighbors for Image3D reproduce 6.5.171's neighpoints[6] order and good flags exactly, so the tie-break rule is preserved. Verified by inspection and by the tie site in the fixture.
• The 38-PR Algorithm Relationship history is real work and accurate on spot-check (BUG: Fix RequireMinNumNeighbors Not Using Ignore Paths #1310, BUG: Fix out-of-bounds array access in RequireMinNumNeighbors Filter #1343, ENH: Remove redundant preflight checks that are already done in the parameter #1535, ENH: Standardize 2D Image Handling #1590 — titles and merge state all correct).
• Path arithmetic (12 of 16) is correct, and uncovered paths are listed rather than omitted.
• Dual-build passes. I ran both configs: 9/9 in simplnx-Rel and 9/9 in simplnx-ooc-Rel.

Blocking

• Non-terminating coarsening loop is reachable from a passing preflight. Algorithms/RequireMinNumNeighbors.cpp:198 — while(counter != 0) only exits when every negative voxel gets filled, but a rejected voxel with no non-negative face neighbour is never filled, so counter never decreases. Repro: 4×1×1, FeatureIds = [-1,-1,-1,-1], NumNeighbors = [0,5], min=3. Preflight passes and -55569 does not fire (feature 1 is active). NX spins forever emitting 4 voxels to update.. (killed at 25 s). 6.5.171 hangs identically, so this is pre-existing and shared — but this PR newly supports and tests negative FeatureIds (path Creating Parameter for "Stacked File List" #12), and this is that path's untested failure twin. Please add a no-progress guard (if an iteration fills nothing, error out or stop with a warning), a test for it, and a Deviation entry.
• vv/deviations/RequireMinNumNeighborsFilter.md says "No behavioral deviation was observed", but this PR introduces at least two deliberate divergences from 6.5.171. (1) Negative FeatureIds: 6.5.171 evaluates activeObjects[featurename] with featurename == -1 (MinNeighbors.cpp:487, out-of-bounds QVector read); NX now skips marking and reassigns from a valid face neighbour — the PR even has a test asserting this. (2) Out-of-range FeatureIds: 6.5.171 indexes out of bounds, NX returns -55567. Both are user-visible, both are bug root cause in 6.5.171 with recommendation trust SIMPLNX. The report's Bug flags | None and the deviations file both contradict the PR's own commit messages ("Moved feature id range check to initial iteration to prevent OOB", "Added defensive checks").
• Circular oracle retained, and de-circularising it is free. RequireMinNumNeighborsTest.cpp:27-53 — the 791-value k_NumberElements array is a snapshot of the filter's own output, with no documented provenance. VV_Notes says such exemplars should be retired in favour of inline data. It does not need to be retired: I verified that k_NumberElements is exactly the input archive's 847-tuple NumElements with 56 entries dropped, order preserved. So the expected array can be derived in-test from the input plus the set of features whose recomputed NumNeighbors < 3, replacing 791 magic numbers with a real Class 1/4 assertion at zero cost.
• Report contradicts itself on status. Header says Status | READY FOR REVIEW; the Summary says "report status remains DRAFT for the outstanding gates listed below" — and no outstanding gates are listed. Pick one and delete the stale sentence.

Report / gate compliance

• Test inventory is missing two TEST_CASEs. Bad Phase Number (line 242) and Phase Array (line 279) exist in the file inside #if 0. The gate requires every TEST_CASE listed and marked kept/new-for-V&V/retired. They reference parameter keys that no longer exist (k_FeatureIds_Key, k_CellDataAttributeMatrix_Key), so they cannot ever compile — delete them and record them as retired (also satisfies the PR checklist's "no commented out code").
• No provenance sidecar for 6_5_test_data_1_v2.tar.gz, and the report's Exemplar archive section gives no SHA512. The archive is consumed by the retained Small IN100 test, so a sidecar at vv/provenance/6_5_test_data_1_v2.md is required. SHA512 is at test/CMakeLists.txt:220 (585b51ba1da9784a…).
• Update the dual-build claim. Oracle section says "both passing in the available in-core build". Both configs pass today (9/9 each) — the gate is satisfiable as written, just say so.
• Oracle section is not reproducible from the report. It does not disclose that the input NumNeighbors differs between the two fixtures ({0,0,3,3} all-phase vs {0,0,0,3} single-phase), and attributes the single-phase result to "nonselected feature 2 remains active" without mentioning that feature 2's input neighbour count was also changed. A reader cannot reconstruct the fixture from the text.
• Name the legacy-comparison fixture and its limits. "Run — matched all five arrays for the oracle fixture" doesn't say which fixture, and the 4×1×1 geometry cannot discriminate vote ties, multi-iteration convergence, X/Y/Z neighbour offsets, or multi-component copies. Either adopt a discriminating fixture or state the limitation explicitly. I have the 6×6×6 fixture, pipelines, reference oracle and outputs staged in ww_work/pr1694_review/ if you want to fold them in.
• No documentation work in the PR (workflow step 10). docs/RequireMinNumNeighborsFilter.md is untouched: the new -55571 preflight error is undocumented, the negative / out-of-range FeatureId behaviour is undocumented, and "coarsened iteratively, one Cell per iteration" needs the non-termination caveat once the guard above lands.

Code review — correctness

• The -55568 guard is itself unsound. Algorithms/RequireMinNumNeighbors.cpp:23:

  if((neighbor >= arraySize || featureIdIndex >= arraySize) && (featureName < 0 && neighbor >= 0 && featureIds.getValue(neighbor) >= 0))

  (a) When neighbor >= arraySize is what triggered the branch, the second clause still calls featureIds.getValue(neighbor) — an unchecked raw index read (DataStore::getValue is m_Data.get()[index]) — so it performs the out-of-bounds access inside the condition meant to report it. (b) arraySize is getSize() (elements = tuples × components) but neighbor/featureIdIndex are tuple indices, so the bound is 3× too loose for CopiedVec3-style arrays. (c) The message prints Num. Tuples: {} but passes the element count. Path [DRAFT] Add parameter for a long multi-line string data. #14 is marked "defensive check" but does not currently work; worth fixing or dropping rather than certifying.
• Error paths leave the DataStructure half-mutated. -55567 fires from inside the marking loop (line 158) after earlier cells have already been set to -1; -55568 and -55570 are worse. Since the range check is a pure read, hoist it into a separate read-only pass over featureIds before any mutation — O(N) reads, and the filter then either errors cleanly or commits.
• Dead variable. int32 neighbor = 0; at line 194 is unused in operator() (the only other neighbor is a distinct local in CopyTupleFromArray). Missed by the "Removed unused variables" commit.
• Redundant state: numFeatures (line 182) is the same value as totalFeatures (line 68); the count loop at lines 278-285 recomputes what numInactiveObjects (line 145) already gives.
• The -5558 warning text reads "…from the feature Attribute Matrix 'DataContainer/CellData/FeatureIds'" — that's the cell array path, not a feature AM. 6.5.171 words it correctly ("If this filter modifies the Cell Level Array …"). In NeighborListRemovalPreflightCode, so it affects several filters.

CPU

• std::fill(voteCount.begin(), voteCount.end(), 0) per rejected voxel is O(numFeatures) where 6.5.171 is O(1). Line 247. 6.5.171 walks the same ≤6 neighbours a second time and zeroes only the entries it incremented. Measured on a 60³ fixture holding the rejected-voxel set constant at 21,600 and varying only the feature-array padding (identical cell output and iteration count in every row):

  numFeatures	voteCount	wall
  51	0.2 KB	0.06 s
  200,001	800 KB	0.13 s
  1,000,001	4 MB	0.41 s
  4,000,001	16 MB	1.46 s

  Cost is O(numRejectedVoxels × numFeatures) and stays hidden only while voteCount is cache-resident. 21,600 rejected voxels is tiny — scaling to a realistic count multiplies the excess directly. Mirroring the legacy reset is a small, local fix.

• Each iteration rescans the entire volume even though the rejected set only shrinks, and re-does the cellDataArrayPaths "move FeatureIds to the end" shuffle plus a getDataAs<IDataArray> lookup per array per iteration. Not blocking, but a worklist would fall out of the guard work above.

Memory / out-of-core

• std::vector<int32> neighbors(featureIds.getNumberOfTuples(), -1) (line 177) is 4 bytes per cell, allocated unconditionally even when nothing is rejected, plus badFeatureIdIndexes at 8 bytes per rejected voxel. That is a hard in-core ceiling for large volumes. The report has no memory section at all — please add one stating the per-cell overhead so users can size jobs.
• OOC storage is not actually exercised. The simplnx-ooc-Rel build passes, but every fixture constructs default in-memory stores and the Small IN100 test loads fully into memory, so "passes in the OOC build" means "compiles and passes there", not "verified out of core". Either add coverage or say so explicitly in the report rather than leaving it implied.

Adjacent (separate issue, not this PR)

• src/simplnx/Utilities/DataGroupUtilities.cpp:45 — for(int32_t i = 1; i < activeObjects.size(); i++) is a signed/unsigned comparison that overflows above 2^31 features. Affects every filter calling RemoveInactiveObjects.

---

Verification artifacts (fixture generator, matched 6.5.171/NX pipelines, NumPy reference oracle, outputs, hang repro, and the voteCount scaling measurement) are staged in ww_work/pr1694_review/ with a ReadMe.md, for OneDrive archival — not committed.

[nyoungbq]

Sorry didn't realize I was here for re-review will review again after its updated