VV: Remove/Extract Flagged Features fully V&V'ed - #1736
Open
imikejackson wants to merge 3 commits into
Open
Conversation
Summary: - Found and fixed 5 bugs in the fill and extract paths (D2 fill loop never terminates when the input has FeatureId 0 cells, the vacated-cell test was `> 0` instead of `< 0`; D3 fill loop never terminates when no vacated cell has a fill source, now error -45436; D4 out-of-range FeatureId indexed the flag vector out of bounds, now error -45435 before any modification; D5 extract ignored sub-filter execute failures and threw on preflight failures, now errors -53901..-53904; D6 a flagged feature with no cells crashed the extract, now warning -53905 and skipped); D1, the first-sighting hang from issue BlueQuartzSoftware#1698, was fixed in v7.4.2 by PR BlueQuartzSoftware#1700 and gains its regression test; - documented 7 deviations from DREAM3D 6.5.171 (D1 first-sighting hang, D2 background-cell hang, D3 no-progress hang, D4 out-of-range FeatureId, D5 swallowed extract errors, D6 empty flagged feature on extract, D7 extracted geometry naming and carried feature Attribute Matrix); - retired 3 tests (fill path never exercised; extract checked only two values); - unit tests replaced with 15 inlined *Class 1 (Analytical) + Class 4 (Invariant)* test fixtures whose CellValue arrays prove the copy source of every filled cell, plus a Small IN100 invariant test that fills 8,535 cells; - added 3 V&V source-tree deliverables (report, deviations, provenance); - removed the duplicate NeighborList preflight warning (-11505; the shared -5558 warning remains), rewrote the filter documentation (fill semantics, background rule, error codes, deviation links), and added the v7.4.2 row to the release-dates table. Removal and fill outputs are bit-identical to DREAM3D 6.5.171 on every fixture; the extract geometry, origin and cell arrays match. Both hangs are reproduced by tests that spin forever on the previous code. Fixes BlueQuartzSoftware#1698 Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
Five scoped review passes (adversarial, senior engineer, CPU, memory, out-of-core) were run on the V&V change set. Findings applied: * Fill loop: measure progress by the number of cells actually filled, not by whether a source was chosen. Listing the Feature Ids array in the ignored arrays still hung the loop; that array is now always copied and the caller is warned (-45438). * Validate that the Feature Ids tuple count equals the geometry cell count (-45437) and reject a feature Attribute Matrix with fewer than two tuples (-45433) before any modification; poll the cancel flag in the validation pass; report the stuck-cell count in the -45436 message. * Replace the per-vacated-cell std::vector tallies with fixed std::array storage, build the cell-array list once per run, and make the neighbor scan's inputs const. * Extract: copy the bounds into a local buffer and delete the temporary bounds array before the first crop, so it is neither copied into every extracted geometry nor left behind on an error return; aggregate the empty-feature warning (-53905) into one message; carry the crop bounds and a further-error count in sub-filter messages; take the bounds store by const reference. * Preflight: remove the dead CreateArrayAction (sized to the cell count) and the deferred delete that the algorithm now performs itself. * Tests: add the Feature-Ids-ignored, tuple-count-mismatch and aggregated warning cases, assert the extracted geometry carries no tempBounds, and add a background-cell fill section to KeepRemoveRankedFeaturesTest, which shares the fill utility. * Docs and V&V report: correct the deviation counts, the tempBounds and ordering claims, add the out-of-core build caveat, drop stale line counts, and document the new codes in both filters' user documentation. Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
…V&V report * Point the legacy-comparison section at the actual archive location (V&V OneDrive library, folder RemoveFlaggedFeaturesFilter) instead of a placeholder name. Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
nyoungbq
approved these changes
Sep 3, 2026
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
2 participants
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.
Summary
Full V&V of
RemoveFlaggedFeaturesFilterper the v2 MTR policy, triggered by the fill-loop hang in #1698. Removal and fill outputs are bit-identical to DREAM3D 6.5.171 on every fixture; the V&V found and fixed six defects and documents seven deviations.FeatureRemovalUtilities, also used by Keep/Remove Ranked Features):>= 0skip). This hung on any input with background cells and fill on, in every DREAM3D-NX release through 7.4.2.-45436instead of spinning (6.5.171 spins). Progress is measured by cells actually filled, and the Feature Ids array is always copied even when listed in Attribute Arrays to Ignore (warning-45438), which closed a second route to the same hang.-45435, tuple count differing from the geometry returns-45437. 6.5.171 reads out of bounds.-53901..-53904instead of being ignored or thrown; a flagged feature with no cells is skipped with one aggregated warning-53905instead of crashing (6.5.171 writes a bogus 1x1x1 geometry at origin -1,-1,-1); the temporary bounds array is copied to a local buffer and deleted before the first crop, so it is neither copied into every extracted geometry nor leaked on error.CreateArrayActionsized to the cell count.CellValuearray so the copy source of every filled cell is asserted, plus a Class 4 Small IN100 invariant test filling 8,535 cells. Both hangs are reproduced by tests that spin forever on the previous code. Keep/Remove Ranked Features gains a background-cell fill test.vv/RemoveFlaggedFeaturesFilter.md(READY FOR REVIEW),vv/deviations/RemoveFlaggedFeaturesFilter.md(D1-D7), provenance sidecar updated, user docs for both filters rewritten, 7.4.2 added to the release-dates table.Five scoped review passes (adversarial, senior engineer, CPU, memory, out-of-core) were run on the change set; the second commit applies their findings. Note for #1568: its Direct/Scanline rewrite of this filter predates these fixes and reintroduces the hangs; see the comment posted there.
Fixes #1698
Test Plan
SimplnxCoreUnitTest '[RemoveFlaggedFeaturesFilter]'passes insimplnx-Relandsimplnx-ooc-Rel(18 cases, run individually under a timeout to prove no hang)[KeepRemoveRankedFeatures]and[RequireMinNumNeighborsFilter]pass in both builds