VV: Group Microtexture Regions full V&V - #1703
Open
imikejackson wants to merge 3 commits into
Open
Conversation
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
force-pushed
the
vv/group_microtexture_regions
branch
6 times, most recently
from
August 24, 2026 21:29
a128f2f to
6d16ae6
Compare
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
force-pushed
the
vv/group_microtexture_regions
branch
from
August 25, 2026 21:21
e6088e6 to
779b2f0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the outstanding gate items left open by the initial V&V cycle (#1637) and promotes the report from
DRAFTtoREADY 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
DRAFTREADY FOR REVIEWThe substantive finding
GroupMicroTextureRegionsships in DREAM3D 6.5.171 as a private filter — listed in_PrivateFiltersinSource/Plugins/Reconstruction/ReconstructionFilters/SourceList.cmakeand registered viaADD_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=falsecomparison target, the provenance sidecar was correct; the report described it incorrectly and the test comment contradicted itself mid-sentence.Verified against the source:
execute()iteratesfor(j = 0; j < groupList.size(); j++)over a list that grows as neighbors are accepted, passinggroupList[j]todetermineGrouping()asreferenceFeature. The local is namedfirstFeaturebut 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:
A secondary item is whether the invariant list in
RandomizeParentIds invariantsis exhaustive.This is a domain/intent judgement about microtexture-region grouping, not a deep code review. Both items are recorded in
vv/provenance/GroupMicroTextureRegionsFilter.mdunder 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
ForceOutOfCoresentinel fixture plus an OOC build configuration.Verification
Full rebuild of preset
NX-Com-Qt69-Vtk96-Rel, thenctest -R GroupMicroTexture→ 5/5 passed, 0 failed (2026-08-10). This supersedes the report's previous 2026-06-11 claim, which predated several merges intodevelop.clang-formatmade no changes to the edited test file.Per
vv_policy.md, promotion toCOMPLETEand theSign-offfield are filled at merge by the approving reviewer.🤖 Generated with Claude Code