VV: Segment Features (C-Axis Misalignment) fully V&V'ed

[imikejackson]

Summary

Full V&V cycle for CAxisSegmentFeaturesFilter per the v2 MTR V&V policy, verified against an independent Class 1 (Analytical) + Class 4 (Invariant) oracle before any legacy comparison.

• Found and fixed 3 bugs, each pinned by a failing-first test:
  • D1 — the shared SegmentFeatures driver burst from an unvalidated raw seed = 0 (introduced in BUG: Fixes issue when segmenting features with a +1 feature count #1466): phantom empty feature / ids shifted by one / growth from a masked voxel whenever index 0 could not legitimately seed. Restored getSeed() for the first seed; fix also covers EBSDSegmentFeatures and ScalarSegmentFeatures, and each consumer suite now carries its own masked-voxel-0 regression pin.
  • D4 — the crystal-structure validation rejected datasets with unindexed (phase-0) or masked-out cells and indexed CrystalStructures without a bounds check (new -8364).
  • D5 — stale getDataAs<ImageGeom> cast crashed on RectGrid input (parameter allows RectGrid; siblings already used IGridGeometry).
• Review hardening: -652/-653 preflight guards (cell arrays / cell AttributeMatrix vs geometry cell count), preflight-consistent FeatureIds path derivation and tuple shape, clean cancel return.
• Retired the 4 circular-oracle exemplar tests (segment_features_test_data CAxis_FeatureIds_* arrays were generated from SIMPLNX output); the archive remains for the EBSD tests. Replaced with 20 tests built on inline pure-Phi Bunge fixtures with the closed-form derivation embedded in the test source.
• Legacy 6.5.171 A/B comparison (official PipelineRunner vs nxrunner, shared legacy-format input): partition-level match on all 5 fixtures (incl. a 3x2x2 masked fixture covering the y/z stride branches) with identical feature counts; bit-identical FeatureIds are unattainable because 6.5.171 always clock-randomizes (D2). 5 deviations documented.
• V&V deliverables: vv/CAxisSegmentFeaturesFilter.md (READY FOR REVIEW), vv/deviations/, vv/provenance/, vv/comparisons/ pipelines + results; user docs updated (hexagonal requirement, mask/phase-0 semantics, RectGrid, deterministic randomization).
• vv_policy.md: "algorithmic choice" root-cause wording aligned with fleet practice (usable outside Rewrite relationships).

Review follow-ups folded in: the shared driver's IsPeriodic option (a silent no-op since the #1373 neighbor refactor dropped #1291's wrap logic) is restored with per-sibling regression pins, and the per-feature 100k-sentinel worklist refill is removed.

Fixes #1690
Fixes #1691

No parameter changes — parametersVersion() stays at 2.

Approving this PR constitutes the second-engineer sign-off for the report; the Oracle section and provenance sidecar flag the two items to double-check (pure-Phi derivation, folded-distance metric).

Test Plan

• OrientationAnalysis::CAxisSegmentFeaturesFilter 20/20 on in-core build
• OrientationAnalysis::CAxisSegmentFeaturesFilter 20/20 on out-of-core build
• EBSDSegmentFeatures 8/8 and ScalarSegmentFeatures 5/5 in both builds (shared-driver regression check, incl. all-cells-masked -87000 and periodic-wrap pins)
• Full OrientationAnalysis:: + SimplnxCore:: suites 1205/1205 on in-core build after the shared-driver changes
• Legacy 6.5.171 A/B comparison rerun post-fix (vv/comparisons/CAxisSegmentFeaturesFilter/results/comparison.md)

[imikejackson]

Second-Engineer V&V Review

Full independent review of the V&V package: every claim in vv/CAxisSegmentFeaturesFilter.md, the deviation entries, and the provenance sidecar was re-verified against source, git history, the legacy 6.5.171 sources and official binary, plus an independent A/B rerun with our own comparison methodology. Verified results first, actionable items after.

Verified — no action needed

• Oracle (both flagged items check out). The pure-Phi derivation was re-derived independently: for quat {sin Φ/2, 0, 0, cos Φ/2} the sample-frame c-axis is (0, ±P·sinΦ, cosΦ), so the dot product between two cells is cos(ΔΦ) regardless of the quaternion P convention — the claimed min(|ΔΦ|, 180−|ΔΦ|) folded metric is exact. Numerically confirmed all four fixture partitions incl. the π-fold pair (w(2°,176°)=174° → folds to 6°). Degree→radian conversion at executeImpl confirmed. The folded metric matches legacy bit-for-bit in structure: v6.5.171 uses the same clamp, acos, and w <= tol || (π − w) <= tol bounds.
• D1 provenance. git show of BUG: Fixes issue when segmenting features with a +1 feature count #1466 confirms it replaced seed = getSeed(gnum, nextSeed) with raw seed = 0; legacy SegmentFeatures::execute() at the v6.5.171 tag gets every seed (including the first) from getSeed(). The restored driver logic is correct for the seed == -1 initial case (clean -87000, no UB; the old raw-0 burst on a 0-dim geometry even hit a % 0).
• D2 provenance. At the v6.5.171 tag: m_RandomizeFeatureIds(true) hard-coded, no filter parameter, clock-seeded RNG — exactly as documented. (The parameter only appears on the 6.5.172 branch, commit 5042ce45f.) The official 6.5.171 binary printed "Randomizing Feature Ids" unconditionally on my rerun, so partition-level comparison is indeed forced.
• Tests. 20 CAxis + 6 EBSD + 3 Scalar all pass at branch head on both NX-Com-Qt69-Vtk95-Rel and simplnx-ooc-Rel. The report's test inventory matches the 20 TEST_CASEs one-for-one; SIMPL 6.4/6.5 conversion fixtures exist; segment_features_test_data.tar.gz is retained for the EBSD tests as stated. parametersVersion() correctly stays at 2 (only parameter description strings changed).
• A/B revalidation. Regenerated the input via make_input.py, re-ran both runners, and compared with an independent partition-canonicalization script: all four fixtures match at the partition level with identical Active tuple counts, and NX ids equal the oracle exactly — the comparison.md table reproduces.
• 3-D parity (gap closed, see action item below). All four committed A/B fixtures are N×1×1, so the legacy comparison never exercised the y/z stride branches. I ran a supplementary 3×2×2 fixture (one feature spanning x-, y-, and z-strides plus a π-fold hop, voxel 0 masked) through both runners: full partition parity (5+1 features both sides) and NX equal to an independently computed flood-fill oracle.

Action items

Correctness / coverage (should address before merge):

• Sibling behavior flip not pinned: all-masked input now errors instead of "succeeding". With the old raw seed = 0 driver, a dataset where no voxel can seed (mask all false, or all phase 0) ran one no-op burst and returned success with one phantom, zero-cell feature for EBSDSegmentFeatures and ScalarSegmentFeatures. Post-fix, the initial getSeed() returns −1 and both now fail with -87000 (EBSDSegmentFeatures.cpp:49-52, ScalarSegmentFeatures.cpp:215-218). This is the correct new behavior (CAxis pins it via Execute Error - No Features Found), but the new sibling regression tests only cover the partially-masked case. Add an all-masked -87000 test per sibling, and consider a one-line note in their eventual deviation docs since this is a user-visible change for pipelines feeding an empty ROI. (addressed in bbbb30a — all-masked -87000 tests added for EBSD and Scalar)
• -87000 message misdiagnoses its only reachable cause. CAxisSegmentFeatures.cpp:101 says "The C-Axis Misorientation Tolerance may be set too low", but tolerance can never produce zero features — every eligible cell seeds at least a singleton. The error is only reachable when no cell is eligible (all masked / all phase 0 / empty geometry). Suggest wording like "No cell was eligible to seed a Feature (all cells are masked out or have a phase of 0)." Same nit applies to the generic driver-path message in the siblings if you touch them for the item above. (addressed in ffcdb49 — message now names the eligible-seed causes in all three filters)

Hardening (recommended, small):

• Cancel-at-entry now mutates data before the first cancel check. SegmentFeatures.cpp:153 calls getSeed() (stamps featureIds[seed] = 1; for CAxis also resizes the feature AM) before the loop-top m_ShouldCancel check at line 164. An if(m_ShouldCancel) { return {}; } above line 153 restores the old "no mutation on pre-canceled entry" property. (addressed in ffcdb49)
• Siblings lack the cancel guard CAxis gained. (addressed in 926f295) On cancel, execute() returns before m_FoundFeatures is assigned, so a direct (non-IFilter) invocation of EBSDSegmentFeatures/ScalarSegmentFeatures misreports -87000 after a cancel. IFilter::execute masks this through the normal API, so low severity — but mirroring the CAxis if(m_ShouldCancel) return {}; guard keeps the three consumers consistent.

Performance:

• New per-cell mask check in the validation loop is OOC-hostile (the one true perf regression). CAxisSegmentFeatures.cpp:57-79 now calls virtual MaskCompare::isTrue() (bounds-checked DataStore::at()) for every unmasked phase>0 cell, roughly doubling the loop's per-element virtual/chunked accesses on masked datasets — tolerable in-core, potentially minutes at 500M cells on a chunked OOC store. Cheap fix: cache validated phases (std::vector<bool> phaseValidated(numEnsembles)) and continue on already-validated phases before touching the mask; optionally break once all ensembles are validated. Reduces the common all-hex case from O(N) mask reads to O(numEnsembles). (addressed in ffcdb49 — validated-phase cache; mask consulted only for the first cell of each phase)
• Per-seed resizeTuples in CAxis getSeed is vestigial and quadratic — consider dropping while the lines are hot. (addressed in 926f295 — getSeed now stamps only, matching EBSD/Scalar; redundant pre-execute Active fill also removed; V&V code-path 13 updated) CAxisSegmentFeatures.cpp:152-158 resizes the feature AM to {gnum+1} on every seed; DataStore::resizeTuples reallocates and copies with no geometric growth, so the Active array pays O(F²) copy traffic plus an AM child-traversal per feature. The siblings (EBSDSegmentFeatures, ScalarSegmentFeatures) only stamp FeatureIds in getSeed and resize once after segmentation — and CAxis operator() already does the authoritative resize + Active refill after execute() returns, so the per-seed resize is dead weight (a legacy-port vestige of updateFeatureInstancePointers()). Pre-existing, but this PR renamed the surrounding lines, and removal aligns CAxis with its siblings.

Docs (trivial):

• Stale sibling test counts in the report. vv/CAxisSegmentFeaturesFilter.md (Test inventory closing paragraph) says "EBSDSegmentFeatures (5/5) and ScalarSegmentFeatures (2/2)" — the suites are 6/6 and 3/3 since the masked-voxel-0 pins landed. The PR description already has the right numbers. (addressed in 00aadce — now 7/7 and 4/4)
• Archive a 3-D fixture in the A/B set. Since the committed comparison only proves 1-D parity, add a TC5-style 3×2×2 masked fixture (features spanning y/z strides) to make_input.py + both pipelines + comparison.md so the archived methodology covers the stride branches. I verified parity for exactly this shape; happy to hand over the fixture values. (addressed in 00aadce — TC5_3D added to make_input.py + both pipelines; full 5-fixture suite rerun 2026-07-24, all partitions match)

Follow-up issues (out of scope for this PR):

• Shared driver re-fills 100k sentinels per feature. (fixed in-PR, 88b4898 — was filed as PERF: SegmentFeatures driver re-initializes the voxel worklist for every feature #1690) SegmentFeatures.cpp:215 voxelsList.assign(size + 1, -1) always runs with size == 0, shrinking the list to 1 so the first accepted neighbor of every subsequent feature triggers a 100k-element resize + explicit −1 refill (~1.6 MB of writes per feature; the sentinels are never read). Affects all three segmentation filters; worth a driver cleanup issue.
• m_IsPeriodic is dead. (fixed in-PR, 88b4898 — periodic wrap restored per ENH: Allow "Segment Features" to consider periodic microstructures #1291 semantics with EBSD/Scalar pins; was filed as BUG: IsPeriodic parameter is ignored by the SegmentFeatures driver #1691) EBSDSegmentFeatures and ScalarSegmentFeatures expose k_IsPeriodic_Key and set m_IsPeriodic, but the driver's neighbor functions never read it — periodic segmentation is silently a no-op. Predates this branch; deserves its own issue since users can set the parameter today.

Adversarial checks that came back clean

Empty-geometry/0-tuple path, nextSeed monotonic-skip invariant, mask-null vs UseMask consistency between the validation loop and getSeed, the −8364 bounds check ordering (cast only after phase > 0), the −651/−652/−653 preflight triangle (a Phases-vs-geometry mismatch cannot slip past preflight), preflight/execute FeatureIds path agreement via getCellDataPath(), quats int64 indexing, voxelsList write-before-grow bounds (safe, no UB), and a subclass sweep (only the three in-repo consumers of SegmentFeatures; all satisfy the stamp-seed-before-return contract the restored first-seed call relies on).