VV: DBSCAN Full V&V Complete

[nyoungbq]

Referenced work file: dbscan_vv is in OneDrive

[imikejackson]

PR 1702 Code Review

Source: review of commit 0c9819be3. Issues grouped by severity.

Fixes for the checked items below are pushed to this branch as aadcecbd0 and 52d8f9273. Verified locally on macOS/arm64 (simplnx-Rel): clang-format read-only gate clean, build clean, ctest -R "SimplnxCore::DBSCAN" 11/11 passing (baseline before the fixes was 10/10). Unchecked items are left for you — each says why.

---

Memory / Lifetime Issues

• QuickSortGrids recursion depth is unbounded, and it is the same risk you just fixed one function below
  File: DBSCAN.cpp (GDCF::QuickSortGrids)
  This PR correctly converts findClusterRoot from recursion to a loop to remove a stack-overflow risk, but the partition recursion right below it still recurses on both halves. When core-grid occupancies arrive already sorted ascending, ProcessSection returns begin every time and depth becomes O(n) — on a large 3D dataset with a few hundred thousand core grids that overflows an 8 MB stack. The comment added in this PR documents the O(n^2) time but not the depth.
  Fixed by recursing into the smaller partition and looping on the larger, which caps depth at O(log n). The two partitions are disjoint, so the order they are processed in cannot affect the result — the LDF exemplar arrays still compare exactly, which the passing 2D/3D tests confirm.

CPU / Algorithm Efficiency

(Deferred — both items change reproducible output for a given seed, so they need a deviation entry rather than a quiet fix. Flagging, not fixing.)

• The Random/SeededRandom shuffle is a biased hand-rolled Fisher-Yates
  File: DBSCAN.cpp (GDCF::cluster)
  Three separate problems: r is drawn from [0, size - 2] because maxIdx is size() - 1 and uniform_real_distribution is half-open, so the last index is never selected as a swap partner; the draw uses the full range instead of [0, i], which is the classic naive-shuffle bias rather than Fisher-Yates; and uniform_real_distribution<float64> + std::floor is both slower and adds floating-point bias where uniform_int_distribution would be exact. std::shuffle(coreGridIds.begin(), coreGridIds.end(), gen) is correct, unbiased and one line.
  Not applied: it changes the grid order produced for a given seed, which changes SeededRandom cluster-ID numbering. That is a user-visible reproducibility change and deserves its own DBSCAN-D<N> entry. Your call.

• ProcessSection uses a first-element pivot, so the O(n^2) case is reachable
  File: DBSCAN.cpp (GDCF::ProcessSection)
  The new comment is accurate. A median-of-three pivot or std::sort with an occupancy comparator would remove it, but either changes the ordering among equal-occupancy grids and therefore the LDF cluster-ID numbering the exemplars pin. Worth doing only alongside an exemplar regeneration. The depth fix above removes the crash risk, which was the more serious half.

Naming Consistency

• case DBSCAN::SeededRandom: was unqualified while its sibling used DBSCAN::ParseOrder::Random
  File: DBSCAN.cpp (GDCF::cluster)
  Both compile because ParseOrder is an unscoped enum, but mixing the two forms in adjacent case labels of one switch reads like a typo. Now both use DBSCAN::ParseOrder::.

• Function-local constants in the test use the k_ global-constant prefix (Pre-existing; no action taken.)
  File: DBSCANTest.cpp (LDFTestCase2D, RandomTestCase2D)
  k_GeneratedIdsPath, k_GeneratedAMPath, k_Seed are locals, not global constants. This is consistent with the rest of the file, so changing it here would only create churn — noting it so it is a deliberate choice rather than an oversight.

Const-Correctness

• findClusterRoot, infer and canMerge were non-const despite only reading state
  Files: DBSCAN.cpp (ClusterForest::findClusterRoot, ClusterForest::infer, GDCF::canMerge)
  ProcessSection and QuickSortGrids in the same class are already const; these three were the outliers. Converting findClusterRoot to an iterative walk in this PR is what made it obviously const-able. All three are now const.

Readability

• case Random: { [[fallthrough]]; } — an empty braced case body wrapping a fallthrough attribute
  File: DBSCAN.cpp (GDCF::cluster)
  Adjacent case labels need no [[fallthrough]] at all; the attribute only exists to suppress the warning when a case has statements before falling through. Wrapping it in its own { } block also puts the null statement inside a nested compound statement rather than directly before the case label, which is exactly the placement compilers disagree about. Apple clang 16 accepts it, but there is no reason to depend on that. Replaced with two adjacent case labels plus a comment explaining why the two orders share one body.

• Copy/paste error in the new memory comment: the 2D class says "on 3D data"
  File: DBSCAN.cpp (HyperGridBitMap2D constructor)
  The comment block was duplicated from HyperGridBitMap3D and the trailing sentence still reads "Small epsilon or active extreme outlier points on 3D data can make this allocation needlessly expensive." Dropped the dimension qualifier since the statement is true for both.

• RunAlgorithm uses "the result has warnings" as a proxy for "the cluster forest is ill-formed"
  File: DBSCAN.cpp (RunAlgorithm)
  if(result.invalid() || !result.warnings().empty()) skips labeling. That is correct today only because -85640 is the single warning cluster() can emit. The first time anyone adds an unrelated warning to cluster() — or to something it calls — labeling silently stops happening and every point comes back 0. An explicit signal (checking the code, or having cluster() report "no forest" separately from "warned") would make it safe. Not fixed because it is a small design decision that is yours to make.

• mergeLRC assigns through a redundant indirection (Pre-existing.)
  File: DBSCAN.cpp (ClusterForest::mergeLRC)
  clusterForestNodes[clusterIdx].parent = clusterForestNodes[lowestClusterIdx].parent; — lowestClusterIdx came out of findClusterRoot, so it is a root and its .parent is always itself. The line is equivalent to = lowestClusterIdx and reads as if it were doing something subtler. Left alone since it is correct and outside this PR's scope.

• clang-format
  Read-only gate run against the repository-root .clang-format on all four changed C++ files at 0c9819be3: clean, no findings. Re-verified after my changes.

UX / Human Interface Guidelines

No UI changes in this PR. The only user-facing string change is the Minimum Points parameter help text, covered under Documentation.

Robustness / Defensive

• An all-false mask leaves the grid bounds NaN, and those NaNs are cast to usize
  Files: DBSCAN.cpp (HyperGridBitMap2D and HyperGridBitMap3D constructors)
  If no point passes the mask, every entry of bounds stays quiet_NaN, so origin is NaN and dims[i] = static_cast<usize>(NaN / spacing) + 2 is undefined behavior. I measured it rather than assuming: arm64 saturates the cast to 0 (dims become 2), x86-64 returns INT64_MIN (dims become 9223372036854775810, whose product then wraps to 8). In both cases the resulting grid happens to come out empty and the filter returns the correct -85640 warning, so — to be clear — this never produced wrong output and I could not make it crash. It is still UB with nonsense intermediates and no guarantee it stays benign under a different compiler or optimization level.
  Guarded explicitly in both constructors, with a message naming the mask as the cause. New fixture F3 pins the contract.

• The -85640 message gives advice that does not apply when the mask is the cause
  File: DBSCAN.cpp (GDCF::cluster)
  "No clusters detected - Consider reducing number of required points (Minimum Points) or increasing acceptable distance (Epsilon)." is good advice for a genuinely sparse dataset and misleading advice when the user's mask excluded everything. My guard emits an explanatory message just before it, which covers the common case, but the warning text itself is still generic. Threading a distinct code out of the constructor is more surgery than a review fix should do.

• The parseOrder switch has no default: label
  File: DBSCAN.cpp (GDCF::cluster)
  A pipeline JSON carrying an out-of-range parse_order_index casts to an enumerator matching no case, and the core grids are then silently left unsorted rather than reporting a bad parameter. Low severity — the GUI cannot produce it — but a hand-edited pipeline can.

• Raw pointer dereference on the cluster Attribute Matrix
  File: DBSCAN.cpp (DBSCAN::operator())
  m_DataStructure.getDataAs<AttributeMatrix>(...)->resizeTuples(...) is a nullptr dereference if the object is missing, which is the exact scenario the try/catch fifteen lines above exists to defend against ("we may be calling this from somewhere else that is NOT going through the normal nx::core::IFilter API"). Per the project guidance this should use getDataRefAs — same assumption, but it throws instead of invoking UB. Converted.

• Raw pointer dereferences on the Attribute Matrix in tests
  File: DBSCANTest.cpp (CheckClusterInvariants, LDFTestCase2D, RandomTestCase2D, 3D test, F1, F2)
  Six getDataAs<AttributeMatrix>(...)->getNumberOfTuples() calls. The project rule is to wrap getDataRefAs in REQUIRE_NOTHROW so a missing object produces a named failure instead of a segfault mid-suite. Converted all six.

Bugs

• The V&V report claims "No SIMPLNX bugs found" while this PR fixes a real one
  Files: vv/DBSCANFilter.md (Bug flags row, Phase 7)
  Commit 131b5d7e8 in this PR is titled "Fix bug in random seed defaulting to user seed", and it is a genuine defect: executeImpl computed a time-based seed for the provenance array but passed filterArgs.value(k_SeedValue_Key) into DBSCANInputValues::Seed. Since k_SeedValue_Key is only linked into the GUI for SeededRandom, Random ran with the default seed 5489 on every single invocation — making it a silent duplicate of SeededRandom. Worth emphasising that docs/DBSCANFilter.md has described Random as "using a time-based seed" since before this PR, so this was a documented-behavior-vs-implementation mismatch, and finding it is exactly what V&V is for. Good catch.
  The problem is only that the deliverable does not say so. I updated the Bug flags row and rewrote Phase 7 to record it. It is not a legacy deviation, so I recorded it in Phase 7 rather than minting a DBSCAN-D<N>; move it if you would rather it were a deviation.

• RandomTestCase2D now asserts an exact cluster-size multiset against a non-deterministic run
  File: DBSCANTest.cpp (RandomTestCase2D)
  A consequence of the seed fix above. For ParseOrder::Random the seed is now time-based, yet the test still requires generatedBins.size() == exemplarBins.size(), an exact match on generatedBins[0] (the noise count), and a one-to-one multiset match of every cluster size against the LDF exemplar — for whatever seed the clock hands it. Cluster membership is order-independent, but border-grid assignment is not: in the expansion loop a border grid reachable from two clusters joins whichever it encounters first, which can move a handful of points between clusters and noise. Before this PR the test was deterministic (it got the user seed, always 5489), so this brittleness is newly introduced.
  I could not make it fail — 6 consecutive runs of the six 2D tests, 36 Random executions, all green — so this is a latent risk, not an observed flake. Partially mitigated: the test now reads the seed back out of the seed array and reports it via INFO, so if CI ever does trip you get the reproducing seed instead of an unreproducible red build. If you would rather remove the risk, drop the exact bin comparison for the Random variant and keep it only for SeededRandom.

• The seed provenance array was never asserted by any test
  File: DBSCANTest.cpp (RandomTestCase2D)
  k_SeedArrayName_Key gets a CreateArrayAction in preflight and is written in executeImpl, and nothing checked either. Now asserted, including that SeededRandom round-trips the user seed unchanged — which is the regression test for the bug this PR fixes.

Documentation

• The report and the deviations file disagree about which legacy binary produced the evidence
  Files: vv/DBSCANFilter.md (Legacy comparison row, Phase 9), vv/deviations/DBSCANFilter.md
  The deviations file is titled "Deviations from DREAM3D 6.5.171" and every entry frames the comparison against 6.5.171, while the report's Legacy comparison row and Phase 9 say the numbers came from 6.5.172. Both cannot be the stated baseline. I deliberately did not rewrite this: other vv/ reports in the repo (ComputeFeatureSizesFilter, FillBadDataFilter, ReplaceElementAttributesWithNeighborValuesFilter) name 6.5.172 openly as a proof-patch build, so there is clearly a house style here and relabeling would have altered a factual provenance claim. Please make the two files agree in whichever direction is right.

• Phase 7 was stale and contradicted both the diff and the At-a-glance table
  File: vv/DBSCANFilter.md (Phase 7)
  It said findClusterRoot recursion was "Deferred — W1 in session notes. No fix in this pass" when the diff fixes exactly that, and its status read "Partially complete — formal review skill not invoked" while the At-a-glance row read "Phases 1-13 complete". Rewritten to record the actual dispositions, including the items intentionally left deferred.

• Counts in the At-a-glance table and code path table are now off by the new fixture
  File: vv/DBSCANFilter.md
  Updated to 11 TEST_CASEs and 18 code paths, added path 4b (mask excludes every point) and the F3 row to the test inventory, and noted in paths 7/8 how the non-deterministic Random path is actually compared.

• Absolute developer-machine paths in a deliverable
  File: vv/DBSCANFilter.md (Phase 9 setup, Phase 12)
  /home/nyoung/DREAM3D-Dev/DREAM3D-Build/D3D-Rel-Develop/Bin/PipelineRunner and /home/nyoung/Apps/DREAM3DNX-Dev/dbscan_vv/ are not reproducible for anyone else. Replaced with non-machine-specific descriptions; the dbscan_vv/ relative references are untouched.

• Minimum Points help text describes point-level semantics the filter does not implement
  File: DBSCANFilter.cpp (parameters)
  "The minimum number of points needed to form a 'dense region'" is the traditional DBSCAN definition. DBSCAN-D1 explicitly lists users who "expect per-point epsilon-neighborhood semantics from the 'minPoints' parameter description" as an affected group — so the description is a named cause of the deviation's user impact, and nothing in the PR fixed it. Reworded to say it is a per-grid-cell occupancy threshold, with a pointer to the new documentation section.

• "Known Differences" slightly overstates the consequence
  File: docs/DBSCANFilter.md
  "Traditional DBSCAN would still cluster these points; GDCF does not" is true for an isolated micro-cluster but not in general — GDCF absorbs such points as border points if either cell is density-reachable from some other core grid. Added that qualification. The rest of the section is a genuinely good addition; the worked ratio and the tuning advice are the right things to tell a user.

• Verified commit is still a placeholder and Status is IN-REVIEW with Sign-off already filled
  File: vv/DBSCANFilter.md (header)
  Expected at this stage given the pending second-engineer review, just flagging so neither is forgotten at deliverable assembly.

Confirmed Correct (no action needed)

• findClusterRoot recursion-to-loop conversion — semantics preserved exactly, including the self-parent base case. The right fix.
• ProcessSection has no out-of-bounds risk despite unguarded front++ / back-- scans. Traced it specifically because it looks dangerous: the pivot value sitting at sorted[begin] halts the first forward scan at begin, and after every swap position back + 1 holds a value >= threshold and front - 1 holds one <= threshold, so each subsequent scan has a sentinel inside the range. It also always returns a value strictly less than end, which is what makes the partition recursion terminate rather than spin on [begin, end].
• canMerge returning false on cancel cannot corrupt output. The early false is indistinguishable from a genuine no-merge, but both call sites re-check m_ShouldCancel at the top of the next iteration and return {}, and the caller discards the DataStructure on cancel. Bounded wasted work, no wrong results. Placing the check in the outer point loop rather than the inner one is also the right call for hot-loop cost.
• Invariant 2 (contiguous IDs 1..maxId) genuinely holds and is not vacuous. gridVoxels only ever contains occupied cells, so every root that survives cleanup labels at least its own grid's points — meaning every renumbered ID 1..N really does appear in the output. It holds for the non-deterministic Random path too.
• preflightImpl switching to getDataRefAs<IDataArray> and dropping the null check is correct: ArraySelectionParameter validates existence and type before preflightImpl runs, and this matches the project guidance. Same for dropping the unused preflightUpdatedValues.
• DBSCANFunctor else-if to early-return conversion preserves behavior exactly, and removing the unreachable trailing return {} is a genuine improvement — the old code had a return after an unconditional error return.
• F1 and F2 derivations check out against the code. Cell side epsilon / sqrt(Dimensions) matches sideLength in both constructors; F1's four unit-square corners really do land in separate cells at epsilon = 0.1; F2's two active points really do share cell 0, and masked points stay 0 because label() does fill(0) and only writes points found in gridVoxels.
• Seed array lifecycle is consistent. The CreateArrayAction in preflightImpl and the write in executeImpl are guarded by the same != LowDensityFirst condition, and k_SeedArrayName_Key is linked for both Random and SeededRandom. No path writes an array that was not created.
• The provenance sidecar is honest about what is not resolved — the 3D exemplar is explicitly still flagged as a circular oracle rather than quietly folded into the "regression fixtures" promotion. That is the right way to write this up.

[imikejackson]

See comments on the PR