VV: Compute Feature Neighborhoods Fully V&V'ed + Enhancements#1652
Merged
imikejackson merged 6 commits intoJul 13, 2026
Merged
Conversation
35 tasks
nyoungbq
approved these changes
Jul 10, 2026
…sed Feature Phases Add a "Search Radius Type" ChoicesParameter to ComputeNeighborhoodsFilter that lets the neighbor search radius be defined either as "Multiples of Average Diameter" (existing behavior, the default) or as an absolute "Search Radius (microns)". The two value inputs are linked to the choice. parametersVersion is bumped 1 -> 2; older pipelines omit the new keys and default to the multiples path, so their results are unchanged. The Equivalent Diameters array is now only used (and only required) in the "Multiples" mode, where it defines the average diameter used for both the radius and the spatial-bin grid size. In the "Search Radius (microns)" mode the radius comes from the user and the bin grid is sized by the search radius itself, so Equivalent Diameters is linked to the "Multiples" choice and hidden/not required. The multiples path is unchanged: binSize = avgDiameter and k = ceil(2*radius/ binSize) reproduce the historical bins and k = ceil(multiples) exactly. Output arrays are anchored on the Centroids array's parent Attribute Matrix (Centroids is required in both modes). Preflight now reports the input Image Geometry info (dimensions/spacing/origin/ units) as a PreflightUpdatedValue so the user has spatial context for choosing a search radius, and in the microns mode warns when the radius is smaller than a single voxel edge or larger than the geometry's largest physical extent. Also remove the Feature Phases input, which was a required parameter but is never used by the algorithm (confirmed unused in legacy DREAM3D 6.5.171 FindNeighborhoods as well), and drop its FromSIMPLJson mapping. Tests: add a hand-verified synthetic-oracle test for the microns mode that supplies NO Equivalent Diameters array (proving it is not required), a preflight info/warnings test (sub-voxel and oversized radius), an equivalence test (microns == multiples when the radius matches), and an invalid-search-radius validation test. Existing exemplar and SIMPL-conversion tests updated for the removed phases parameter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ull V&V V&V of ComputeNeighborhoodsFilter against a Class 1 analytical oracle, then a legacy DREAM3D 6.5.171 comparison on Small IN100, surfaced a regression in the prior rewrite (PR BlueQuartzSoftware#1485): the neighbor search radius had been changed to a single global 'avgDiameter * multiples / 2', which at the default multiples=1 found ~37x fewer neighbors than legacy (mean 0.29 vs 10.93). Fix: restore a per-feature Euclidean radius 'equivalentDiameter[i] * multiples' for the Multiples mode, keeping the rewrite's legitimate box->sphere improvement (true distance test) while recovering legacy's per-feature scale. Post-fix the filter correlates 0.894 with legacy and finds 50.8% as many neighbors - exactly the sphere/box volume ratio (a sphere is the geometrically correct region). The average diameter is now used only to size the internal spatial-hash grid. The "Search Radius (microns)" mode is unaffected (constant radius, symmetric). Tests: MultiplesAnalyticalOracle rewritten as a per-feature Class 1 oracle that also checks the per-feature asymmetry; SyntheticOracle (microns) unchanged; InvalidSearchRadius rebuilt on synthetic input. Retired the three tests that depended on the circular 'compute_feature_neighborhoods' exemplar (Neighborhoods_1, Neighborhoods_3, SearchRadiusMicrons) - replaced by the independent analytical oracle plus the legacy comparison. Removed the now-unused archive download. Docs updated to describe the per-feature radius and the "Average" naming caveat. V&V deliverables added: - src/Plugins/SimplnxCore/vv/ComputeNeighborhoodsFilter.md (report) - src/Plugins/SimplnxCore/vv/deviations/ComputeNeighborhoodsFilter.md (D1 box vs sphere; D2 Feature Phases removed) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, add small search-sphere figure
Rather than embed a heavy full-page infographic PNG (~550 KB), keep the
explanatory content as markdown text (searchable, themeable, diffable) and use a
single focused, to-scale figure for the one genuinely-visual element: the search
sphere around a feature at 1x/2x/3x its own diameter.
- Add the per-feature search-radius formula and the "no division by two" /
"average only sizes the grid" clarifications to the Description text.
- New figure ComputeFeatureNeighborhoods_SearchSphere.svg (editable source)
rendered to a ~120 KB PNG, following the ComputeGroupingDensity_Infographic.
{svg,png} convention (docs embed the PNG; Sphinx only renders SVG for HTML).
The figure is drawn to scale: the 1x ring radius equals the feature's own
diameter (rings scale 1:2:3), so just-touching neighbors sit on the 1x ring.
- Remove the oversized full-page infographic and the now-obsolete, inaccurate
ComputeFeatureNeighborhoods_MultiplesOfAvgDiameter.png.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Line-wrap the "Search Radius Context" preflight value construction and a test comment to satisfy clang-format. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two CI failures introduced by this branch: 1. FilterValidation "Filter Parameter Keys": ChoicesParameter keys must end in "_index" (matching transformation_type_index, init_type_index, etc.). Rename the new key "search_radius_type" -> "search_radius_type_index". Unit tests reference the k_SearchRadiusType_Key constant, so only the string literal changes. 2. PY::OrientationAnalysis pipeline tests (Morphological Statistics and the downstream Crystallographic/mesh/GBCD pipelines that read its output): the shipping "(03) Small IN100 Morphological Statistics.d3dpipeline" still carried the v1 ComputeNeighborhoods args (feature_phases_array_path present; no search_radius_type_index / search_radius). Update the node to the v2 parameter set (drop feature_phases, add search_radius_type_index=0 and search_radius=1.0, bump parameters_version to 2). Verified the updated node loads and executes cleanly via nxrunner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes from adversarial review of this PR: * Exclude feature 0 (background) as a search source: the parallel range now starts at feature 1 and microns mode no longer assigns feature 0 a search radius. Previously Neighborhoods[0] could be polluted (microns mode gave feature 0 a full radius; multiples mode counted coincident centroids), violating the count==list-size invariant at index 0. Neighborhoods[0] is now always 0 with an explicit empty list. * Remove the removed feature_phases_array_path argument from the 01_Small_IN100_Morphological_Statistics.py example (registered ctest; the pybind wrapper throws on unknown kwargs). * Average diameter used to size the bin grid now divides by the number of real features (excluding background) instead of all tuples. * Pin the inclusive distSq <= radiusSq boundary with an exact radius-distance fixture pair (exactly representable in float32) and document the strict-< vs inclusive-<= difference vs 6.5.171 in D1. * Add error-path tests asserting -5730/-5731/-5732/-5733 codes; add deviation D3 for the new non-positive radius/multiplier rejections. * Assert Neighborhoods[0]==0 and empty NeighborhoodList[0] in both analytical oracles (multiples fixture places feature 0 coincident with feature 1 so a regression is detected). * Guard the parallel scan with requireStoresInMemory for the Centroids store per the project thread-safety policy. * Rename the "Multiples of Average Diameter" display strings to "Multiples of Equivalent Diameter" to match the actual per-feature semantics (JSON keys unchanged); update docs and remove the apology note. Fix the misleading "A x B x C voxels" preflight radius string. * Update the V&V report: code-path table recounted (13 of 14 exercised, only the cancel path uncovered), path 12 claim now matches the code. Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
imikejackson
force-pushed
the
feature/neighborhoods-search-radius-type
branch
from
cc2f1af to
d10fb34
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
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 the Compute Feature Neighborhoods filter (
ComputeNeighborhoodsFilter), including a result-changing bug fix to the default "multiples" mode, a new absolute search-radius mode, removal of a never-used input, background-feature hardening, and preflight guidance. Deliverables:vv/ComputeNeighborhoodsFilter.md,vv/deviations/ComputeNeighborhoodsFilter.md(D1–D3), analytical-oracle test suite.Bug fix: per-feature search radius restored (result-changing)
PR #1485 correctly replaced the legacy bin-box test with a true Euclidean distance test, but also redefined the radius as a single global
avgDiameter × multiples ÷ 2. LegacyFindNeighborhoodsscales the reach by each feature's own equivalent diameter with no÷ 2. At the defaultmultiples = 1the broken version found ~37× fewer neighbors than legacy (mean 0.29 vs 10.93 on Small IN100).This PR restores
radius_i = equivalentDiameter[i] × multiples(per-feature, no division by two). On Small IN100 atmult = 1the mean neighbor count moves from 0.29 to 5.55; the remaining ~52% ratio vs legacy's 10.93 is exactly the sphere-vs-box volume ratio and is documented as deviation D1. The average diameter is now used only to size the internal spatial-hash grid.Changes
New: "Search Radius Type" selection
A linkable
ChoicesParameterselects how the radius is defined:radius_i = equivalentDiameter[i] × multiples(per-feature, asymmetric relation possible).Equivalent Diametersnot required).Background feature 0 excluded from the computation
Feature 0 is never a search source and never a candidate;
Neighborhoods[0]is always 0 with an emptyNeighborhoodList[0]. Both oracles assert this (the multiples fixture places feature 0's centroid coincident with feature 1's, so a regression fails the test).Removed: "Feature Phases" input
Required by legacy
dataCheck()but never referenced in the computation (true in both legacy and NX). Parameter,FromSIMPLJsonmapping, and the stale usage in the01_Small_IN100_Morphological_Statistics.pyexample (a registered ctest) are removed. Output arrays anchor on theCentroidsparent Attribute Matrix. Documented as D2.Preflight guidance and validation
-5734) or geometry-spanning (-5735) radii.-5732) / radius (-5733) at preflight — legacy silently produced all-zero output. Documented as D3.Backward compatibility
parametersVersionbumped1 → 2. Missing keys fill with defaults on load; the removedFeature Phaseskey produces a harmless warning.(03) Small IN100 Morphological Statistics.d3dpipelinemigrated toparameters_version: 2.Testing
5 TEST_CASEs, all passing in-core (
SimplnxCore::ComputeNeighborhoods*):ComputeNeighborhoods_SyntheticOracle— Class 1 hand-derived oracle, microns mode, 7 features, incl. an exact radius-distance pair pinning the inclusivedistSq <= radius²boundary and feature-0 exclusion assertions. Runs with no Equivalent Diameters array (proves it is not required in microns mode).ComputeNeighborhoods_MultiplesAnalyticalOracle— Class 1 hand-derived oracle for the per-feature multiples mode, incl. the asymmetry case that distinguishes per-feature from global radius, and feature-0 exclusion assertions.ComputeNeighborhoods_InvalidSearchRadius— 4 sections asserting exact error codes-5730/-5731/-5732/-5733.ComputeNeighborhoods_SearchRadiusPreflightInfo— geometry-info value + sub-voxel/oversized warnings.Retired (see V&V report test inventory):
ComputeNeighborhoods_1,ComputeNeighborhoods_3(circular exemplar oracles, invalidated by the radius fix),ComputeNeighborhoods_SearchRadiusMicrons(premise invalid under per-feature radii).Test Plan
Notes for reviewers / V&V
The V&V report enumerates 14 code paths (13 exercised; only the cancel path uncovered). Deviations D1 (sphere vs box + inclusive boundary), D2 (Feature Phases removal), D3 (non-positive parameter rejection) are in
vv/deviations/ComputeNeighborhoodsFilter.md.