VV: Compute Feature Phases Full V&V

[nyoungbq]

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 Adversarial Review — ComputeFeaturePhasesFilter

Full adversarial review of the code, tests, and the three vv/ deliverables, plus an independent re-run of the legacy A/B comparison (the committed deviations table was treated as a claim to falsify, not as evidence). Verdict at the bottom.

Independent verification results (all pass)

• ✅ All 9 ComputeFeaturePhasesFilter tests pass on the in-core build (macOS ARM, Release)
• ✅ All 9 tests pass on the out-of-core build
• ✅ Error codes verified against source: -5351 / -5355 (DataArrayUtilities.cpp:184-195), -61860 / -61861 (algorithm); both new codes are unique in the codebase
• ✅ D1's "always 0" premise verified: CreateArrayAction → CoreDataIOManager constructs Int32DataStore with initValue = 0, so the never-written index 0 (and any gap features) are deterministically zero
• ✅ Core loop semantics verified identical to 6.5.171 FindFeaturePhases::execute() for featurePhases[1..N]: conflict detection against the first phase seen per feature, last-cell-wins assignment

A/B re-run (fresh evidence, not a re-read of the deviations table)

Legacy-native inputs for Cases A/B/C were minted fresh and run through the DREAM3D 6.5.171 PipelineRunner (FindFeaturePhases) and this branch's nxrunner (ComputeFeaturePhasesFilter):

Case	6.5.171 FeaturePhases	SIMPLNX FeaturePhases	[1..N]	index 0
A	[0, 1, 1, 2, 2]	[0, 1, 1, 2, 2]	bit-identical	equal
B	[0, 2, 1, 2, 2]	[0, 2, 1, 2, 2]	bit-identical	equal
C	[2, 1, 2, 1]	[0, 1, 2, 1]	bit-identical	6.5.171 = 2, SIMPLNX = 0 → D1 confirmed

The deviations file's 2026-07-10 comparison table is reproduced exactly. A/B testing claim: validated.

---

Blocking — factual errors in the V&V deliverables

• D4 mischaracterizes 6.5.171. vv/deviations/…D4 states: "SIMPL: Single generic message with no feature ID list." The 6.5.171 source (FindFeaturePhases.cpp, warning block) emits one line per conflicting feature: "Phase Feature <id> created <N> warnings." — it lists feature IDs and per-feature conflict counts. Confirmed empirically: the Case B legacy run printed Phase Feature 1 created 1 warnings. Rewrite D4 as: SIMPLNX consolidates into a single warning, truncates at 15 IDs, and drops the per-feature conflict counts (also note: 6.5.171 can warn for feature 0; SIMPLNX never does).
• D3 mischaracterizes 6.5.171. The entry states: "SIMPL: No check; mismatch would result in an out-of-bounds read." 6.5.171 dataCheck() calls validateNumberOfTuples() over FeatureIds + CellPhases and errors out at preflight time. The real deviation is when (preflight vs. execute) and the error code — not the presence of the guard. Rewrite accordingly.
• Wrong branch attribution. Deviations header says the hardening landed on "branch vv/fill_bad_data, 2026-07-10". It landed in this PR's commit 47f04a1 on vv/compute_feature_phases. Also resolve the internal contradiction: the header says "All four deviations were introduced in post-Phase-9 algorithm cleanup" while D4's body says "Introduced at port time (PR #1455)".
• "Open items: None" is false. vv/provenance/… records "Second-engineer oracle review: Not yet conducted." Either conduct it (the file itself argues it is a <5-minute derivation) or list it as an open item in the report's At-a-glance table.
• Cancel-path wording. Report path 3 says cancel is "Not tested (cancel-signal injection not supported)". Per project V&V convention the cancel path is intentionally untested (meaningful mid-flight cancellation would require a second thread, and the mechanism is known-working); reword the report to say that rather than "not supported" — IFilter::execute does accept a shouldCancel flag (IFilter.hpp:233), so "not supported" is not the accurate rationale. No test required.
• Test-count inconsistency. Summary and Oracle sections say "7 inline" tests; At-a-glance and provenance say 8. There are 8 new TEST_CASEs (Cases 1–8). Fix the "7"s.
• Exemplar-archive claim is stale. Report says 6_6_stats_test_v2.tar.gz is still "Use[d] for … SIMPL Backwards Compatibility test". That test reads local test/simpl_conversion/*.json fixtures — after retiring (Valid Parameters), no test of this filter touches the archive (it stays in test/CMakeLists.txt for other filters). Correct the report and provenance rows.
• Cross-doc test-name drift. Deviations refer to "Background Feature Skip", "Inconsistent Phase Warning", "Warning Truncation"; actual TEST_CASE names are Valid: Feature 0 Skip, Valid: Mixed Phase Warning, Valid: Truncated Mixed Feature Warning. Align the names.
• Verified commit: pending — fill in before merge.

Algorithm — correctness-adjacent

• Negative phase on background cells is not caught. The -61861 guard (ComputeFeaturePhases.cpp:63) runs after the featureId == 0 skip (line 56), so cellPhases = -5 on a background cell passes silently. The new docs paragraph ("halts with an error if any cell phase value is negative") overstates. Either move the check above the skip, or scope the docs/D2 text to "cells belonging to a valid feature (featureId > 0)".
• Size-mismatch check belongs in preflight. Tuple counts are static metadata; checking featureIds vs. cellPhases in preflightImpl restores parity with 6.5.171 (which fails this at dataCheck time) and gives users the error before execution. Keep the execute-time guard if desired, but preflight should catch it first.

Senior-engineer nits (code)

• ComputeFeaturePhases.cpp:83 — warnStr is built unconditionally even when there are no conflicts; construct it inside the count != 0 branch.
• Lines 90/104 — magic number 15 appears twice; hoist constexpr usize k_MaxListedFeatures = 15;.
• Line 107 — "and N more occurrence(s)": the count is of features, not occurrences; "and N more feature(s)" is accurate. Also "Affected Phase Features:" reads oddly — "Affected Feature IDs:" is clearer. (Warning text only; Case 3/4 assertions must be updated in lockstep.)
• Lines 68–78 — initialFeaturePhase[featureId] indexes with a raw int32 while line 78 uses static_cast<usize>(featureId); make them consistent. storedPhaseId can be const.
• Line 30 comment — the validator enforces non-negative feature ids (0 is allowed and then skipped), not "all positive".
• No progress messaging in the per-cell loop, and m_ShouldCancel is loaded every iteration; on large volumes the filter is silent. Consider the ThrottledMessenger/MessageHelper pattern. Optional follow-on: track last-seen phase in the local vector and bulk-write featurePhases once after the loop — removes the per-cell setValue into the store entirely (also friendlier to OOC stores).

Senior-engineer nits (tests)

• Validation-section getDataRefAs<Int32Array>() calls (Cases 1, 2, 3) are not wrapped in REQUIRE_NOTHROW() — project testing convention requires it for every getDataRefAs.
• Error tests (Cases 5–8) omit UnitTest::CheckArraysInheritTupleDims(dataStructure); — convention says every TEST_CASE.
• New TEST_CASEs omit UnitTest::LoadPlugins(). Harmless here (filters are constructed directly), but inconsistent with the suite convention — confirm intentional or add it.
• Case 3's assertion warnMsg.find("Features: 1") is brittle against message rewording (see warning-text nit above); prefer asserting the exact ID list segment after any text change.
• Case 4 asserts the truncation suffix but not that exactly 15 IDs are listed — cheap to add ("1, 2, …, 15" substring or count commas).
• Suggested additional cases: (a) cancel path (see blocking item), (b) gap feature — a feature index with no cells stays 0 (currently untested; documents the zero-init contract D1 relies on), (c) negative phase on a background cell (locks in whichever resolution is chosen for the docs/behavior gap above).

Suggested surgical patch — DREAM3D 6.5.171 (root-cause proof only)

D1 is the only deviation that changes an output array, and the only patch worth minting. Applied to a local build of the legacy source (proof tooling, not a shipping fix — feature 0 is invalid by convention, so the 6.5.171 output is not demonstrably wrong):

// FindFeaturePhases::execute(), inside the per-cell loop
gnum = m_FeatureIds[i];
if(gnum == 0)  // match SIMPLNX: background cells do not contribute a phase
{
  continue;
}

Expected proof: Case C FeaturePhases[0] becomes 0, and the A/B diff goes to zero across all three cases. D2/D3/D4 are error/warning-path only — no output arrays to reconcile, no patches warranted.

---

Verdict

Algorithm is a faithful port with sensible hardening; featurePhases[1..N] is independently confirmed bit-identical to 6.5.171 across all three deviation-file cases, and D1 reproduces exactly as documented. The blocking list is entirely documentation accuracy in the vv/ deliverables (two deviations mischaracterize the legacy behavior, plus internal contradictions) and one wrong testability claim. Fix those, decide the background-negative-phase question, and this is ready to sign off.

[imikejackson]

See PR comments