Skip to content

VV: Group Microtexture Regions full V&V - #1703

Open
imikejackson wants to merge 3 commits into
BlueQuartzSoftware:developfrom
imikejackson:vv/group_microtexture_regions
Open

VV: Group Microtexture Regions full V&V#1703
imikejackson wants to merge 3 commits into
BlueQuartzSoftware:developfrom
imikejackson:vv/group_microtexture_regions

Conversation

@imikejackson

Copy link
Copy Markdown
Contributor

Closes the outstanding gate items left open by the initial V&V cycle (#1637) and promotes the report from DRAFT to READY FOR REVIEW.

No SIMPLNX algorithm source changed in this PR. The only non-documentation edit is a corrected comment in one test fixture.

What changed

Item Before After
Status DRAFT READY FOR REVIEW
Legacy comparison "Not run" (blamed the RNG seed) N/A — no public legacy equivalent
Code path coverage "9 of 9 exercised" (contradicted the table) 7 of 9, in both the dashboard row and the section
D2 / D3 affected users overstated 6.5.171 migration impact corrected
OOC verification listed as outstanding out of scope for this round, recorded as a decision

The substantive finding

GroupMicroTextureRegions ships in DREAM3D 6.5.171 as a private filter — listed in _PrivateFilters in Source/Plugins/Reconstruction/ReconstructionFilters/SourceList.cmake and registered via ADD_SIMPL_FILTER(... FALSE ...), which the legacy source 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 an A/B against 6.5.171 is not merely impractical — it is impossible. That is a stronger and more accurate justification than the non-reproducible-RNG argument the report previously gave.

This propagated: D2 and D3 both claimed migration impact on 6.5.171 users who could never have run the filter. Both corrected. D3 now also explains why a bug introduced in 2014 survived to discovery — it lived in a filter nobody could reach.

Deliverables disagreed with each other

On the UseRunningAverage=false comparison target, the provenance sidecar was correct; the report described it incorrectly and the test comment contradicted itself mid-sentence.

Verified against the source: execute() iterates for(j = 0; j < groupList.size(); j++) over a list that grows as neighbors are accepted, passing groupList[j] to determineGrouping() as referenceFeature. The local is named firstFeature but is the current BFS frontier feature, not the seed. Grouping is therefore the transitive closure of the pairwise-tolerance relation along neighbor chains. Report and test comment now match the sidecar.

The Tolerance Boundary fixture was additionally re-derived under all three possible seed orders (getSeed() draws randomly among unparented features); F1, F2, and F3 each yield the partition {F1,F2}, {F3}, so the assertion is seed-order independent.

What a reviewer is actually being asked

One question, and it is about intent, not mechanics:

Grouping is the transitive closure of the tolerance relation. At 10° tolerance, features at Φ = 0°, 8°, 16° all merge into a single group even though the end members are 16° apart. This matches legacy GroupFeatures::execute — but is it the intended user-facing semantic, or an inherited accident?

A secondary item is whether the invariant list in RandomizeParentIds invariants is exhaustive.

This is a domain/intent judgement about microtexture-region grouping, not a deep code review. Both items are recorded in vv/provenance/GroupMicroTextureRegionsFilter.md under Second-engineer oracle review.

Out of scope

OOC verification. Not required by the MTR SBIR deliverable and owned separately by another engineer. Recorded in the report as a deliberate exclusion rather than an open gap. Closing it would require a ForceOutOfCore sentinel fixture plus an OOC build configuration.

Verification

Full rebuild of preset NX-Com-Qt69-Vtk96-Rel, then ctest -R GroupMicroTexture5/5 passed, 0 failed (2026-08-10). This supersedes the report's previous 2026-06-11 claim, which predated several merges into develop. clang-format made no changes to the edited test file.

Per vv_policy.md, promotion to COMPLETE and the Sign-off field are filled at merge by the approving reviewer.

🤖 Generated with Claude Code

@imikejackson imikejackson changed the title VV: Group MicroTexture Regions — READY FOR REVIEW VV: Group Microtexture Regions full V&V Aug 10, 2026
imikejackson added a commit to imikejackson/simplnx that referenced this pull request Aug 18, 2026
Only BlueQuartzSoftware#1701 landed since the last refresh; it normalizes report statuses
and does not move the MTR closure, which stays at 25/29.

* Mark Phase 0 item 4 done: all 34 reports on develop now lead with one
  of DRAFT / READY FOR REVIEW / COMPLETE (31/2/1)
* Record BlueQuartzSoftware#1702 (DBSCAN) and BlueQuartzSoftware#1703 (GroupMicroTextureRegions) as open and
  awaiting first review, neither affecting the closure
* Add a third gap to the attestation section: BlueQuartzSoftware#1703 reports that the
  earlier GroupMicroTextureRegions cycle claimed 9 of 9 code paths while
  its own table showed 7, alongside a mis-stated legacy comparison and
  an overstated migration impact — defects a diff-focused review misses,
  so vv_status.py should cross-check the dashboard against the tables
* Correct the all-branches versus develop gap from 11 points to 4

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
@imikejackson
imikejackson force-pushed the vv/group_microtexture_regions branch 6 times, most recently from a128f2f to 6d16ae6 Compare August 24, 2026 21:29
imikejackson and others added 3 commits August 25, 2026 17:21
Closes the outstanding gate items left open by the initial V&V cycle
(BlueQuartzSoftware#1637) and promotes the report DRAFT -> READY FOR REVIEW.

Summary:
- Confirmed no new bugs (D1/D2 were found and fixed in the BlueQuartzSoftware#1637 cycle;
  no SIMPLNX source change required by this pass);
- documented 0 new deviations from DREAM3D 6.5.171, but corrected the
  existing 3 (D1 default-mode error, D2 randomization disabled, D3 legacy
  phase1 bug): the legacy comparison is reclassified "Not run" -> "N/A —
  no public legacy equivalent" because GroupMicroTextureRegions ships in
  6.5.171 as a _PrivateFilter (ADD_SIMPL_FILTER(... FALSE ...)), compiled
  but not instantiable from a pipeline, so no A/B is possible; D2 and D3
  "affected users" were overstated on that basis and are now accurate;
- no tests retired (fresh gate-closing pass, not a re-verification);
- augmented the existing 5 inlined *Class 1 (Analytical) + Class 4
  (Invariant)* fixtures with no new cases; corrected a self-contradictory
  comment in the Tolerance Boundary fixture and documented that its
  expectation is seed-order independent;
- added 3 V&V source-tree deliverables (report, deviations, provenance) —
  already present from BlueQuartzSoftware#1637, updated in place by this pass;
- fixed a coverage-gate violation: "9 of 9 paths exercised" contradicted
  two rows marked "Not directly tested"; corrected to 7 of 9 in both the
  dashboard row and the Code path coverage section.

Notes for the reviewer:
- The three deliverables disagreed on the UseRunningAverage=false
  comparison target. The provenance sidecar was correct; the report and
  the test comment were not. Verified against the source that
  determineGrouping() receives groupList[j] — the current BFS frontier
  feature, not the seed — so grouping is the transitive closure of the
  pairwise-tolerance relation along neighbor chains. Report and test
  comment now match the sidecar.
- OOC verification is deliberately out of scope for this round: not
  required by the MTR SBIR deliverable and owned separately by another
  engineer.
- Status stops at READY FOR REVIEW; per vv_policy.md the promotion to
  COMPLETE and the Sign-off field are filled at merge by the approving
  reviewer.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parameter defaults table claimed SIMPLNX and DREAM3D 6.5.171 both
default UseRunningAverage to false. Legacy does; SIMPLNX has defaulted it
to true since the initial port. That flag selects the comparison target,
so a pipeline migrated on defaults does not produce the same regions.

* Correct the defaults table: legacy false, SIMPLNX true.
* Add deviation D4 covering the default divergence, its effect on
  grouping, and the recommendation to set the flag explicitly rather
  than rely on either default. D4 is independent of D3, which concerns
  which Laue classes are validated once the running average is enabled.
* Fix the filter documentation, which described the running average as
  off by default. It is on by default; the legacy default was off, and
  migrating users are told so.
* Update the Algorithm Relationship row to count three intentional
  differences rather than two.

The default itself is left at true: both comparison targets are intended,
and external review confirmed the running average is the appropriate
target for ensemble-orientation work.

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
Records the full-dataset legacy comparison for GroupMicroTextureRegions
and closes the outstanding A/B item.

* Default UseSeed to true. With UseRunningAverage enabled the grouping
  is order-dependent, so a clock-derived seed made the shipped
  configuration irreproducible: two identical runs on the same input
  differed on 194658 of 3710475 touching pairs. Seeding by default
  makes repeated runs deterministic.

* Add a "Legacy comparison (DREAM3D 6.6.x)" section. 6.5.171 could not
  be used because the filter is unregistered there and cannot be
  instantiated at all, so 6.6.x is the only legacy line in which it is
  runnable. Upstream processing was computed once in SIMPLNX and
  exported to legacy v7 format so both implementations ran only this
  filter on byte-identical input: an alpha/beta titanium scan reduced
  to 1840838 features with 106731 hexagonal/cubic touching pairs.

  With UseRunningAverage=false the two produce identical partitions
  (746877 groups, zero differing pairs). With it enabled the
  differences resolve entirely into RNG seed order, which is inherent
  to the algorithm and reproducible within either implementation on its
  own, and deviation D3. Surgically patching both causes out of a local
  legacy build reduced the disagreement to zero pairs at seed 1234 and
  two at seed 5489, the latter consistent with float rounding at the
  acceptance threshold.

* D3 now carries the production-data measurement: 907 incorrect
  cross-Laue merges, structural rather than stochastic, all removed by
  the one-line phase1 correction. D2 and D4 note the new UseSeed
  default and the order-dependence it addresses.

Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
@imikejackson
imikejackson force-pushed the vv/group_microtexture_regions branch from e6088e6 to 779b2f0 Compare August 25, 2026 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant