Skip to content

VV: Group Microtexture Regions full V&V #1703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
imikejackson wants to merge 1 commit into
base: develop
Choose a base branch
from
+47 −18
Open

VV: Group Microtexture Regions full V&V #1703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/Plugins/OrientationAnalysis/test/GroupMicroTextureRegionsTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,12 +393,21 @@ TEST_CASE("OrientationAnalysis::GroupMicroTextureRegionsFilter: Class 1 Analytic
using namespace AnalyticalFixtures;

// 3 real features on a chain F1 -- F2 -- F3. F1 c-axis at Phi=0, F2 at Phi=8, F3 at Phi=20.
// - F1 -- F2 : 8 deg -- under 10 deg tolerance -> GROUP
// - F2 -- F3 : 12 deg -- over 10 deg tolerance using F2's c-axis (since UseRunningAverage=false,
// the algorithm compares each candidate to the BFS seed's c-axis, but
// inside the BFS walk over already-grouped features, comparison is from
// THAT feature's c-axis, not the original seed's) -> DO NOT BRIDGE
//
// With UseRunningAverage=false, determineGrouping() computes the reference c-axis from its
// referenceFeature argument, which execute() supplies as groupList[j] -- the current BFS frontier
// feature, NOT the seed and NOT a running average. (The local is named firstFeature, but groupList
// grows as neighbors are accepted, so it advances past the seed.) Grouping is therefore the
// transitive closure of the pairwise-tolerance relation along neighbor chains.
//
// - F1 -- F2 : 8 deg, compared from F1's c-axis -- under 10 deg tolerance -> GROUP
// - F2 -- F3 : 12 deg, compared from F2's c-axis -- over 10 deg tolerance -> DO NOT BRIDGE
//
// Expected: 2 distinct groups -> {F1, F2}, {F3}.
//
// This expectation is independent of which feature getSeed() happens to pick first (it draws
// randomly among unparented features). Seeding at F1, F2, or F3 all produce the same partition,
// because the only bridging edge (F2--F3) exceeds tolerance when evaluated from either end.
FixtureData td = CreateScaffold(/*numFeatures=*/4);

SetAvgQuat(td, 1, QuatFromPhiDeg(0.0f));
Expand Down
26 changes: 16 additions & 10 deletions src/Plugins/OrientationAnalysis/vv/GroupMicroTextureRegionsFilter.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,21 @@
| DREAM3D 6.5.171 equivalent | `GroupMicroTextureRegions` — `Source/Plugins/Reconstruction/ReconstructionFilters/GroupMicroTextureRegions.{h,cpp}` (inherits from `GroupFeatures` base class) |
| Patched-legacy reference | 2025-10-23 surgical fix to a local build of the legacy source (`BUG: GroupMicrotextureRegions bug fixes, expose as usable filter`) — used here as a corroborating reference for port-time fixes |
| Verified commit | *<filled at SBIR deliverable assembly>* |
| Status | DRAFT |
| Sign-off | *Michael Jackson <mike.jackson@bluequartz.net> — V&V pending review* |
| Status | READY FOR REVIEW |
| Sign-off | *pending second-engineer review* (V&V authored by Michael A. Jackson <mike.jackson@bluequartz.net>) |

## At a glance

| Aspect | Current state |
|------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| Algorithm Relationship | **Port** (with deliberate inheritance flattening and 2 port-time **regressions fixed during this V&V cycle**, plus 1 latent legacy bug already corrected in the original 2024-01-08 port). Legacy is a `GroupFeatures` subclass; SIMPLNX inlines the `GroupFeatures::execute` BFS loop into a single algorithm class. UUID retained: `5e18a9e2-…` → `3f695987-…`. |
| Oracle (confirmed) | **Class 1 (Analytical) primary + Class 4 (Invariant) companion** — pure Bunge φ1=0, Φ=φ°, φ2=0 fixtures make the c-axis angular distance between any two features exactly \|Φ_A − Φ_B\| (folded into [0°, 90°]); expected groupings derive in closed form from the chosen tolerance and neighbor adjacency. |
| Code paths enumerated | 9 of 9 (line-by-line walk of `GroupMicroTextureRegions::operator()`, `execute`, `getSeed`, `determineGrouping`, `randomizeParentIds`) |
| Code paths enumerated | **7 of 9 exercised** (9 enumerated by a line-by-line walk of `GroupMicroTextureRegions::operator()`, `execute`, `getSeed`, `determineGrouping`, `randomizeParentIds`). The two uncovered paths (#3 null-list guard, #6 background-phase skip) are defensive branches, listed with reasons in the Code path coverage table. |
| Tests today | 5 test cases: Class 1 "Pure-Phi Bunge" (5 features, 3 groups), Class 4 "RandomizeParentIds invariants" (equivalence-class preservation + seed determinism), Class 1 "Tolerance Boundary" (3 features, 2 groups), defect-A regression pin (UseNonContiguousNeighbors=false runs without error), SIMPL 6.4+6.5 backwards-compat (DYNAMIC_SECTION). |
| Exemplar archive | **None — data inlined in test source** (`test/GroupMicroTextureRegionsTest.cpp` namespace `AnalyticalFixtures`). Scaffold helper builds an `{nX,1,1}` ImageGeom + Cell/Feature/Ensemble AMs; per-feature pure-Phi quats and per-feature neighbor lists are set explicitly per test. |
| Legacy comparison | **Not run.** Legacy 6.5.171 randomizes parent IDs by default with a non-reproducible seed; bit-identical comparison is not meaningful. A 2025-10-23 fix applied to a local build of the legacy source exposes `RandomizeParentIds` as a user parameter (default false) — the same design SIMPLNX now uses post-fix; this V&V verifies SIMPLNX against an independent oracle. |
| Legacy comparison | **N/A — no public legacy equivalent.** In DREAM3D 6.5.171 `GroupMicroTextureRegions` is a **private** filter: it is listed in `_PrivateFilters` in `Source/Plugins/Reconstruction/ReconstructionFilters/SourceList.cmake` and registered via `ADD_SIMPL_FILTER(... FALSE ...)`, which the legacy source itself documents as "available from other filters but the user will not be able to use them from the DREAM3D user interface." It is therefore compiled but not instantiable from a pipeline, so no A/B run against 6.5.171 is possible. A fix applied to a local build of the legacy source promotes it to public and exposes `RandomizeParentIds`; that patched build is the corroborating reference for D1–D3. Secondary obstacle: legacy randomizes parent IDs from a clock-derived seed, so even once exposed it yields permutation-equivalent, never bit-identical, arrays. |
| Bug flags | **None remaining.** Two port-time regressions (D1, D2) were found and fixed during this V&V cycle and are pinned by tests. One legacy 6.5.171 bug (D3) was already corrected in the original 2024 SIMPLNX port and is documented for migration users. |
| V&V phase | Phases 1, 3, 4, 5, 6, 7, 8, 9, 11 — complete. **Outstanding:** OOC build verification (dual-build protocol), Phase 13 (Status promotion to READY FOR REVIEW), second-engineer review of the oracle design and the patched-legacy reference fix. |
| V&V phase | Phases 1, 3, 4, 5, 6, 7, 8, 9, 11 — complete. Legacy comparison closed as **N/A** (private filter in 6.5.171 — see the Legacy comparison row). OOC verification **out of scope for this round** — not required by the MTR SBIR deliverable and owned separately by another engineer; additionally GMTR is serial with no OOC dispatch variant and no `ForceOutOfCore` sentinel in its tests, so an OOC build would exercise in-memory stores and prove nothing. Store-agnosticism is established by inspection instead (see Test inventory). **Outstanding:** second-engineer review of the oracle design — specifically the chain-transitive grouping characteristic (see Oracle § Second-engineer review). |

## Summary

Expand Down Expand Up @@ -94,11 +94,15 @@ Companion assertions, applicable independent of which feature is picked as the f

### Second-engineer review

*Pending.* The c-axis closed-form derivation for pure-Phi Bunge angles is sibling-shared with `ComputeFeatureNeighborCAxisMisalignmentsFilter`, whose Class 1 oracle was reviewed previously; the same derivation applies here. A second-engineer pass on (a) the patched-legacy reference fix as the corroborating source for the D2 design, and (b) the on-the-boundary 12°-rejection assumption in Fixture B (i.e., that under `UseRunningAverage=false` the algorithm compares each candidate against `firstFeature`'s c-axis, not against the running seed's c-axis), is recommended before Status promotion.
*Pending.* The c-axis closed-form derivation for pure-Phi Bunge angles is sibling-shared with `ComputeFeatureNeighborCAxisMisalignmentsFilter`, whose Class 1 oracle was reviewed previously; the same derivation applies here.

The specific item a second engineer should confirm is the **chain-transitive grouping characteristic** under `UseRunningAverage=false`. In `execute()` the BFS iterates `for(j = 0; j < groupList.size(); j++)` over a list that *grows* as neighbors are accepted, and passes `groupList[j]` to `determineGrouping()` as `referenceFeature`. The local variable is named `firstFeature`, but it is the **current frontier feature, not the seed** — so each candidate is compared against its immediate BFS predecessor, not against the seed and not against a running average. Grouping is therefore the transitive closure of the pairwise-tolerance relation along neighbor chains: with a 10° tolerance, features at Φ = 0°, 8°, 16° would all merge into one group even though the end members are 16° apart. **Is that the intended semantic?** It matches legacy `GroupFeatures::execute`, but it is a behaviour users should understand, and it is the one place where a wrong oracle would silently confirm a wrong implementation.

Fixture B (`Class 1 Analytical (Tolerance Boundary)`; Φ = 0°/8°/20°, tol 10°) was hand-checked against **all three possible seed orders** — `getSeed()` picks randomly among unparented features, so the fixture must not depend on which feature seeds first. Seeding at F1, F2, or F3 all yield the same partition {F1,F2}, {F3}, because the only bridging edge (F2↔F3, 12°) exceeds tolerance from either direction. The assertion is therefore seed-order independent.

## Code path coverage

*9 of 9 paths exercised.*
*7 of 9 paths exercised.* Paths #3 and #6 are defensive/low-value branches and are listed below with their reasons; they are counted as uncovered rather than folded into the total.

Source: `src/Plugins/OrientationAnalysis/src/OrientationAnalysis/Filters/Algorithms/GroupMicroTextureRegions.cpp` (266 lines).

Expand Down Expand Up @@ -127,7 +131,9 @@ Logical phases: **(a) per-call init** in `operator()` (RNG seed, parent-ID init)
| `OrientationAnalysis::GroupMicroTextureRegionsFilter: SIMPL Backwards Compatibility` | kept | DYNAMIC_SECTION over SIMPL 6.4 + 6.5 conversion fixtures. Validates UUID and argument-key conversion only; not a behavioral test. Was the only passing test under the old `[.][UNIMPLEMENTED][!mayfail]` regime. |
| *(retired)* `OrientationAnalysis::GroupMicroTextureRegionsFilter: Valid Filter Execution` (tag `[.][UNIMPLEMENTED][!mayfail]`) | retired | Old test used empty `DataPath{}` arguments throughout; could not pass preflight and was tagged hidden/expected-fail. Replaced by the two `Class 1 Analytical` tests above. The replacement actually exercises the algorithm with real data. |

All non-retired tests pass on `vv/group_microtexture_regions` (verified on the in-core release build at 2026-06-11). OOC verification: pending — this V&V cycle did not run the OOC build profile, but the algorithm reads `Int32Array`, `Float32Array`, `UInt32Array`, and `NeighborList<int32>` via reference-binding in the constructor; OOC-incompatible patterns (raw-pointer access, parallel writes to the same array) are not used.
All 5 non-retired tests pass in the in-core build — **re-verified 2026-08-10** on preset `NX-Com-Qt69-Vtk96-Rel` after a full rebuild (`ctest -R GroupMicroTexture` → 5/5 passed, 0 failed, 0.15 s). This supersedes the earlier 2026-06-11 spot-check, which predated several upstream merges into `develop`.

**OOC: not run, by design.** `GroupMicroTextureRegions` is a serial algorithm with no out-of-core dispatch variant (no `DispatchAlgorithm` split, no Scanline implementation, no bulk `copyIntoBuffer`/`copyFromBuffer` path), so `SIMPLNX_TEST_ALGORITHM_PATH` has nothing to switch for it. Its tests also carry no `UnitTest::PreferencesSentinel(DataStorageMode::ForceOutOfCore, ...)`, and sentinel-less tests remain in-memory even inside an OOC build — so running the existing suite under an OOC configuration would exercise in-memory stores and prove nothing. Store-agnosticism is therefore established **by inspection**: the algorithm binds `Int32Array&`, `Float32Array&`, `UInt32Array&`, and `NeighborList<int32>&` and reads them only through the public `AbstractDataStore` API (`getValue`/`setValue`/`operator[]`); it takes no raw pointers into store memory and performs no parallel writes, the two patterns that break under a disk-backed store. Note that `IParallelAlgorithm::requireArraysInMemory()` is **not** applicable here — it only toggles parallelization off when arrays are OOC-backed, and this algorithm is serial. **Performance caveat (not a correctness gap):** the grouping BFS walks neighbor lists in random feature order, which is chunk-cache hostile; a disk-backed run on a large feature set would be slow even though it would be correct. **Out of scope for this V&V round, by decision (2026-08-10):** OOC functionality is not required by the MTR SBIR deliverable, and out-of-core support across the filter set is owned separately by another engineer. Closing the gate would mean adding a `ForceOutOfCore` sentinel fixture plus an OOC build configuration; that work is deliberately excluded here rather than left undone by oversight, and it does not gate this report.

## Exemplar archive

Expand All @@ -139,8 +145,8 @@ The fixture scaffold (`AnalyticalFixtures::CreateScaffold(numFeatures)`) builds

## Deviations from DREAM3D 6.5.171

Direct array-by-array comparison against 6.5.171 is not meaningful: 6.5.171 randomizes parent IDs by default using a clock-derived seed (irreproducible) and produces *grouping equivalence classes* that match SIMPLNX's groups under any permutation, but never *bit-identical* arrays. The three documented deviations are design-level statements rather than per-array diffs.
Direct array-by-array comparison against 6.5.171 is **not possible**, for a structural reason: `GroupMicroTextureRegions` ships in 6.5.171 as a **private** filter (`_PrivateFilters` in `Source/Plugins/Reconstruction/ReconstructionFilters/SourceList.cmake`, registered `ADD_SIMPL_FILTER(... FALSE ...)`). Private filters are compiled but, in the legacy source's own words, "the user will not be able to use them from the DREAM3D user interface" — they are not registered for pipeline use, so `PipelineRunner` cannot instantiate the filter and no A/B fixture can be run. Even setting that aside, 6.5.171 randomizes parent IDs from a clock-derived seed, so it yields grouping equivalence classes that match SIMPLNX under permutation but never bit-identical arrays. The three deviations below are therefore design-level statements derived from source inspection and from a local build of the legacy source with the filter promoted to public — not per-array diffs.

- `GroupMicroTextureRegionsFilter-D1` — Defect A: pre-fix SIMPLNX returned error `-99345` when `UseNonContiguousNeighbors=false` (filter unusable in default mode). **Fixed.** See `vv/deviations/GroupMicroTextureRegionsFilter.md`.
- `GroupMicroTextureRegionsFilter-D2` — Defect B: SIMPLNX never randomized parent IDs (legacy 6.5.171 always randomized; a fix applied to a local build of the legacy source exposes `RandomizeParentIds` as a user parameter, default false). **Fixed by exposing `RandomizeParentIds` (default false) + restoring the helper and plumbing the user seed through.** See `vv/deviations/GroupMicroTextureRegionsFilter.md`.
- `GroupMicroTextureRegionsFilter-D3` — Legacy 6.5.171 bug: when `UseRunningAverage=true`, `phase1` is never assigned and the Hex_High acceptance check silently fails — no features ever group. Bug introduced upstream by J. Tucker on 2014-01-30 (commit `7e49e52f` in original DREAM3D). SIMPLNX corrected this in the 2024-01-08 initial port; the same fix was applied to a local build of the legacy source. **Documented for migration users.** See `vv/deviations/GroupMicroTextureRegionsFilter.md`.
- `GroupMicroTextureRegionsFilter-D3` — Legacy 6.5.171 bug: when `UseRunningAverage=true`, `phase1` is never assigned and the Hex_High acceptance check silently fails — no features ever group. Bug introduced upstream by J. Tucker on 2014-01-30 (commit `7e49e52f` in original DREAM3D). SIMPLNX corrected this in the 2024-01-08 initial port; the same fix was applied to a local build of the legacy source. **Note:** because the filter was private in 6.5.171, this bug was not reachable by 6.5.171 end users — it is documented for provenance and for anyone who built the legacy filter themselves, not as a migration hazard. See `vv/deviations/GroupMicroTextureRegionsFilter.md`.
Loading
Loading