VV: Multi-Threshold Objects

[mmarineBlueQuartz]

• Add tests for untested code coverage.
• Add V&V documentation.
• Fix bug regarding thresholding over ThresholdSets.
• Fix bug where inverting mask values does not work.

Naming Conventions

Naming of variables should descriptive where needed. Loop Control Variables can use i if warranted. Most of these conventions are enforced through the clang-tidy and clang-format configuration files. See the file simplnx/docs/Code_Style_Guide.md for a more in depth explanation.

Filter Checklist

The help file simplnx/docs/Porting_Filters.md has documentation to help you port or write new filters. At the top is a nice checklist of items that should be noted when porting a filter.

Unit Testing

The idea of unit testing is to test the filter for proper execution and error handling. How many variations on a unit test each filter needs is entirely dependent on what the filter is doing. Generally, the variations can fall into a few categories:

• 1 Unit test to test output from the filter against known exemplar set of data
• 1 Unit test to test invalid input code paths that are specific to a filter. Don't test that a DataPath does not exist since that test is already performed as part of the SelectDataArrayAction.

Code Cleanup

• No commented out code (rare exceptions to this is allowed..)
• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)
• Added test (or behavior not changed)
• Updated API documentation (or API not changed)
• Added license to new files (if any)
• Added example pipelines that use the filter
• Classes and methods are properly documented

[imikejackson]

Full V&V review of this PR: every claim in the V&V documents was re-verified against head 909bff118, plus an adversarial algorithm review, a CPU/memory review, and an independent legacy 6.5.171 A/B run (which this PR's own deviations doc lists as outstanding).

Independent A/B validation (run for this review)

Shared legacy-format input (100 tuples, Int32 = 0..99, Float32 = 0.01*(i+1)); PipelineRunner 6.5.171 vs this branch's nxrunner vs an independent numpy oracle:

Case	Config	Legacy filter	Result
AB1	Int32 > 42 AND Float32 < 0.70 (flat)	MultiThresholdObjects	MATCH (legacy = NX = oracle)
AB2	Int32 > 20 AND (Float32 < 0.60 OR Int32 == 55) (nested set)	MultiThresholdObjects2	MATCH
AB3	Int32 < 80 OR NOT(Int32 > 30 AND Float32 < 0.95) (inverted nested set)	MultiThresholdObjects2	MATCH

Same three configs at 50M random tuples: this branch matches the oracle exactly. All 17 ctest entries pass locally.

The same pipelines run against develop (pre-PR): AB2 yields an all-false mask (38/100 wrong) and AB3 differs in 51/100 values. Both bug-fix claims in the PR description are real and the fix restores legacy semantics — legacy's invertThreshold() flips values element-wise; the old NX std::reverse was a misport, and the old per-item functor dispatch broke nested-set combination entirely.

Also confirmed from legacy source: MultiThresholdObjects2::dataCheck() rejects non-scalar arrays (error -11003), so component-index selection is NX-only.

Actionable items

Correctness / behavior

• Restore the cancel check. operator()() no longer checks m_ShouldCancel anywhere (the member is now unused). This reverts the cancel check deliberately added by ENH: Add missing cancel checks to lots of filters #1582 and makes large thresholding runs uncancellable. A per-threshold check inside ThresholdSet's loop (or per-chunk) would restore it.
• Restore the SIMPL backwards-compatibility test. TEST_CASE "SIMPL Backwards Compatibility" was deleted, reverting coverage re-enabled by BUG: Fix SIMPL JSON conversion segfault and re-enable backwards-compatibility checks #1605. The 6.4/6.5 fixtures still exist under test/simpl_conversion/ but nothing exercises them now — and the V&V report cites them as evidence.
• Restore valid-execution coverage for custom TRUE/FALSE values. TEMPLATE_TEST_CASE "Valid Execution - Custom Values" (10 types) was deleted; only the out-of-bounds/boolean error paths remain. The rewritten InsertThreshold combines via == trueValue / == falseValue, so custom values interact directly with the new logic and currently have zero execution coverage.
• Decide the fate of ErrorCodes::UnequalComponents (-4001) (MultiThresholdObjectsFilter.hpp:40) — dead after the preflight component-count check removal. Remove it or mark it retired (removing changes the public enum; either way the report should note the relaxed preflight behavior).

V&V document corrections (each verified against head)

• Bug flags: None identified is wrong — this PR itself fixes two user-facing bugs, confirmed by the A/B above (develop produces an all-false mask for a plain nested set). Document both bugs in the report, with a note for users of earlier NX releases whose nested-set masks were silently wrong.
• Legacy comparison is no longer outstanding — fold in the A/B results above (flat vs MultiThresholdObjects, nested + inverted vs MultiThresholdObjects2, all match at head). The deviations file can move past "comparison not yet run"; expected entry count stays 0 for these configs.
• Resolve the open question on component-index: legacy Advanced rejects non-scalar arrays (-11003), so it had no equivalent — the feature is NX-only.
• Status fields are inconsistent: header says READY FOR REVIEW, Sign-off says pending — DRAFT, At-a-glance lists "promotion DRAFT → READY FOR REVIEW" as outstanding.
• Test inventory lists every test as kept/new but omits the two removed TEST_CASEs (Valid Execution - Custom Values, SIMPL Backwards Compatibility). Removals must appear as retired with a reason.
• Stale note: k_MismatchingComponentsArrayPath "leftover at line 31 worth deleting" — it no longer exists on this branch.
• "Mask DataType — 11 SECTIONs, one per mask-output DataType": the TEST_CASE has 10 SECTIONs (boolean is not a SECTION; it's covered via defaults elsewhere, as the same sentence says).
• Material-PRs-since-baseline should include ENH: Add missing cancel checks to lots of filters #1582 (cancel checks — reverted here) and BUG: Fix SIMPL JSON conversion segfault and re-enable backwards-compatibility checks #1605 (SIMPL conversion test — deleted here).
• Code-path table should enumerate the custom TRUE/FALSE execution path (currently uncovered per above), so the "23 of 24" figure is accurate after the fix.

CPU / memory

Measured on 50M tuples (3 threshold filters per pipeline, Release): develop 18.2s / 774MB peak RSS → this PR 21.9s / 861MB. Filter-only time roughly +50%; peak temp usage is now (nesting depth + 2) full-size stores vs 1 std::vector before.

• Drop the functor-level temp + copy loop (ThresholdSetFunctor, Algorithms/MultiThresholdObjects.cpp:197-216). ThresholdSet already produces the complete result; pre-fill the output store with falseValue and pass it directly — saves one N-size allocation and two full passes.
• The std::fill in ThresholdValue (line 147) is redundant — ThresholdFilterHelper writes every element.
• Hoist the if(inverse) per-element branch out of InsertThreshold's loop.
• Temps moved from std::vector to AbstractDataStore — every hot-loop access is now a virtual call, which is where the measured slowdown comes from. If OOC-capable temps are the goal, consider bulk/chunked access; per-element operator[] on a chunked HDF5-backed temp will thrash. Also worth confirming temp stores created via DataStoreUtilities::CreateDataStore under force-large-data prefs are cleaned up.

Test quality / cleanup

• Dead debug locals in CheckThresholdSet2 (test cpp 607-608): value and expected computed then ignored.
• maskArray dereferenced without a null check (test cpp 1133-1134); project convention is REQUIRE_NOTHROW + getDataRefAs.
• 6 of 9 TEST_CASEs are missing UnitTest::CheckArraysInheritTupleDims(dataStructure) (project testing convention) — all the new Valid Single Thresholds / Threshold Sets / Input Array DataType cases.
• operator()() leftovers: firstValueFound only feeds a constant !false, and the thresholdSet local (line 249) is unused.
• Doxygen on InsertThreshold / ApplyThresholdValues is stale (parameter names no longer exist; "threshould" typo).

Verified good

• UUID mapping (both legacy UUIDs → this filter), FromSIMPLJson dual-converter branching, filter UUID, and the recursive component-index preflight check all match the report.
• 9 TEST_CASE groups / 17 ctest entries, all passing locally; CI green on all platforms.
• Class 1 analytical oracle with in-memory fixtures and no exemplar archive — complies with the no-circular-oracle policy; the test matrix genuinely enumerates operator x invert x union x nesting x both DataType axes.
• The algorithm restructure itself is semantically correct against both legacy filters (A/B above), including the forced-OR seeding of each set's accumulator.

[imikejackson]

See comments

[imikejackson]

Re-review at head c563a3764 (branch was rebased since the last pass at 909bff118, plus 63a685aab "V&V Fixes" and f722bb901 "Update V&V docs"). Built and ran the suite locally in an in-core Release build: 28/28 SimplnxCore::MultiThresholdObjects* ctest entries pass.

Most of the last round is genuinely fixed — see "Resolved" at the bottom. Three items below are new or newly-quantified and should block; the rest are carry-overs and nits.

---

Blocking

• B1 — The V&V docs misdate the D1/D2 fix and cite a commit that exists in no published history. Both vv/MultiThresholdObjectsFilter.md and vv/deviations/MultiThresholdObjectsFilter.md state the bugs were "fixed by commit 25f1986f1 … which predates this V&V pass" / "fixed pre-branch" / "Status: retired 2026-04-23", and advise that "anyone on a pre-25f1986f1 build should upgrade". Verified:

  git merge-base --is-ancestor 25f1986f1 upstream/develop  -> NO
  git merge-base --is-ancestor 25f1986f1 <PR head>         -> NO   (pre-rebase object, now unreachable)
  git patch-id: 25f1986f1 == 6d137ff30   (6d137ff30 is on this branch, NOT on develop)
  

  The fix has never shipped — it ships with this PR. Please reword D1/D2 status to "fixed by this PR" (or cite the post-merge SHA at deliverable assembly), drop the "pre-25f1986f1 build" upgrade advice in favor of "any release prior to the one containing this PR", and note that the Verified commit header field is still a placeholder.

• B2 — Valid Execution, Mask DataType (10 SECTIONs, ctest ENH: Allow Naming STL AttributeMatrices #568) asserts nothing about the filter's output. k_TupleCount was reduced 20 -> 5 in this PR, but checkMaskValues() (test/MultiThresholdObjectsTest.cpp:906-925) kept the hardcoded if(i < 5) split, so the true-value branch is unreachable. The float fixture is 0.01…0.05 compared against > 0.05, so the expected mask is all-false — and the mask array is zero-initialized at creation. Demonstrated by stubbing MultiThresholdObjects::operator()() to return {};: every other valid-execution test fails, and ENH: Allow Naming STL AttributeMatrices #568 still passes. On develop this test was meaningful (20 tuples, 10 false / 10 true). Fix by choosing a threshold that splits the 5-tuple fixture and deriving the split from k_TupleCount rather than a literal 5; the stale comment above the loop still describes the old 20-tuple / 0.1 fixture. Note this test is the report's only cited coverage for code-path row 22.

• B3 — The analytical oracle disagrees with the implementation for fractional thresholds on integer arrays, and the fixture size hides it. ThresholdFilterHelper::filterDataWithComparision does T value = static_cast<T>(m_ComparisonValue) (Algorithms/MultiThresholdObjects.cpp:85), so 5.5 becomes 5 for an int32 array, while ExpectedIntSingleComponentMask compares in double. Valid Single Thresholds: Int GENERATEs 5.5 precisely to probe this, but at k_TupleCount = 5 the array only holds 0…4, so the divergent element never exists. Demonstrated by bumping k_TupleCount to 8: that test then fails for <, ==, and != at threshold 5.5. Legacy performs the identical cast (SIMPL/Source/SIMPLib/Filtering/ThresholdFilterHelper.h:60), so the implementation is correct and the oracle helper is wrong. Please model the truncation in the Expected*Mask helpers and record the comparison-value truncation as verified, legacy-matching behavior in the Oracle section — otherwise any later fixture-size increase will look like a regression.

Carry-overs still open from the last round

• Doc counts are stale: report says "9 TEST_CASE groups / 17 ctest entries" (lines 17, 19, 72) and the deviations file repeats "17" (lines 13, 28). Actual is 11 groups / 28 ctest entries (ctest -N -R MultiThresholdObjects).
• Test inventory table still has 9 rows — add SIMPL Backwards Compatibility and Valid Execution - Custom Values now that both are restored.
• "11 SECTIONs" for Valid Execution, Mask DataType (report line 129) is still wrong — there are 10, and there is no boolean SECTION. Same for code-path row 22 (line 107) claiming "boolean + … all 11 types".
• Stale note at report line 114: k_MismatchingComponentsArrayPath "leftover at line 31" — the constant no longer exists on this branch.
• Code-path table still omits the custom TRUE/FALSE execution path; rows 6–7 cover only the preflight bounds errors. It now has real coverage (ctest API: Euclidean Geometry Updates #571–580) and deserves a row so the "23 of 25" figure is accurate.
• CPU/memory item 1 — drop the functor-level temp + copy loop (Algorithms/MultiThresholdObjects.cpp:234-241). ThresholdSet already produces the complete result; pre-filling the output store and passing it in directly saves one N-size store and one full pass, and drops peak temps from (depth + 2) to (depth + 1).
• CPU/memory item 3 — hoist the if(inverse) branch out of InsertThreshold's per-element loop (Algorithms/MultiThresholdObjects.cpp:30).
• CPU/memory item 4 — temps are still accessed via per-element operator[] on AbstractDataStore (virtual dispatch), which is where the previously measured ~+50% filter time came from. Confirmed these temps really can be OOC-backed under force-large-data prefs (SimplnxOoc/src/SimplnxOoc/StoreFactory.cpp, createChunkedStoreUnique), so chunk thrashing is a real risk, not hypothetical. (Item 2, the redundant std::fill, is fixed.)
• Dead code in operator()(): firstValueFound (line 273) only ever feeds !false, and thresholdSet (line 276) is unused.

New nits

• ThresholdValue has an unused totalTuples local (line 159), and its leading comment ("create and initialize an array with FALSE") no longer describes what the function does.
• int32_t& err is threaded through ThresholdValue / ThresholdSet / ThresholdSetFunctor and is never written or read. Pre-existing on develop, but this rewrite is the natural place to drop it.
• ThresholdSet's template <typename T> is vestigial — both call sites instantiate it with bool only.
• ThresholdSetFunctor's temp store is not fill(false)-ed (line 234) while ThresholdSet's is (line 190), and it is then read through the forced-Or path in InsertThreshold. Benign today — both the core store (CoreDataIOManager.cpp) and the OOC store (StoreFactory.cpp) zero-initialize — but the asymmetry is a trap; add the fill(false) for consistency.
• Report line 110 defines the D1 trigger as a set mixing a leaf with a nested set; the "Missing" note at line 132 defines it as "a top-level set whose only child is a single nested set, no siblings". Those are different shapes — AB2 is the mixed one. Pick one.
• Valid Execution - Custom Values is missing UnitTest::CheckArraysInheritTupleDims(dataStructure).
• In the == / != SECTIONs, the second Check… call re-asserts the same data under the complementary operator (test/MultiThresholdObjectsTest.cpp:381-388 and the Float / Multi-Component equivalents). That is a tautology of the helper, not additional coverage of the filter.
• Removing UnequalComponents = -4001 from the public ErrorCodes enum is an API break for any downstream plugin referencing it (nothing in this repo does). Worth a report note alongside the now-relaxed preflight behavior, which the report body does not currently mention.
• Report line 80 says the algorithm is "~255 lines"; the file is 288.

Highest-value remaining addition

• Add the in-repo regression test for the D1 trigger shape (code-path row 25). Confirmed still absent: every CreateThresholdSet* helper passes either all leaves or all nested sets to setArrayThresholds(), never a mix. Since D1 is a real user-facing bug that this PR fixes, this is the single most valuable test left.

Resolved since the last review

Cancel check restored (shouldCancel threaded through ThresholdSet's per-threshold loop and ThresholdSetFunctor); SIMPL Backwards Compatibility and Valid Execution - Custom Values both restored and passing; ErrorCodes::UnequalComponents removed with no dangling references in-repo; redundant std::fill in ThresholdValue removed; Doxygen on InsertThreshold / ApplyThresholdValues now matches the real parameters and the "threshould" typo is gone; dead debug locals in CheckThresholdSet2 removed; maskArray null check added; CheckArraysInheritTupleDims added to the new Valid Single Thresholds / Threshold Sets / Input Array DataType cases; docs now document both bugs, fold in the legacy A/B, resolve the component-index question as D3, list #1582 and #1605 as material PRs, and the status fields are internally consistent.

---

Environmental note, unrelated to this PR: a stale libSimplnxReview.simplnx in the build's Bin/ directory fails all 28 entries with -32 : Duplicate UUIDs found in SIMPL UUID maps! UUID: 5e18a9e2-e342-56ac-a54e-3bd0ca8b9c53 Plugin: SimplnxReview. The 28/28 result above is with that plugin moved aside.

[imikejackson]

Inline anchors for blocking items B1-B3 from the re-review comment above.

[imikejackson]

B1 (blocking) — this commit reference is unreachable and the timeline is wrong.

25f1986f1 is an ancestor of neither develop nor this PR head; it is the pre-rebase object, now dangling:

git merge-base --is-ancestor 25f1986f1 upstream/develop  -> NO
git merge-base --is-ancestor 25f1986f1 c563a3764         -> NO
git patch-id: 25f1986f1 == 6d137ff30   (6d137ff30 is on this branch, NOT on develop)

So the D1/D2 fix does not predate this V&V pass — it ships with this PR, and has never been in a release. That also makes "anyone on a pre-25f1986f1 build should upgrade" (D1 and D2 Recommendation sections) unactionable for a user.

Please reword to "fixed by this PR" (or cite the post-merge SHA at deliverable assembly time), change the D1/D2 Status fields off "retired 2026-04-23 … prior to this V&V pass", and phrase the upgrade advice as "any release prior to the one containing this PR". Same wording appears in the report's Bug flags row and Summary. The report's Verified commit header field is also still a placeholder.

[imikejackson]

B2 (blocking) — this makes Valid Execution, Mask DataType (10 SECTIONs, ctest #568) assert nothing about the filter's output.

This PR reduced k_TupleCount 20 -> 5, but the split here is still the hardcoded 5 from the old 20-tuple fixture, so the loop runs i = 0..4 and the else branch (the only place a TRUE value is checked) is unreachable. The float fixture is 0.01…0.05 compared against > 0.05, so the expected mask is all-false — and the mask array is already zero-initialized at creation.

Net effect: the test cannot fail on any output defect. Confirmed by stubbing MultiThresholdObjects::operator()() to return {}; — every other valid-execution test fails, and #568 still passes. On develop this test was meaningful (20 tuples, 10 false / 10 true).

Fix: pick a comparison value that actually splits the 5-tuple fixture, and derive the split from k_TupleCount instead of a literal. The comment two lines up still describes the old 20-tuple / 0.1 fixture and needs updating too. Worth noting the V&V report cites this test as the only coverage for code-path row 22 (mask DataType dispatch).

[imikejackson]

B3 (blocking) — the analytical oracle disagrees with this line, and the fixture size hides it.

The comparison value is truncated to the input array's type here, so a threshold of 5.5 becomes 5 for an int32 array. ExpectedIntSingleComponentMask in the test file instead compares in double. Valid Single Thresholds: Int GENERATEs 5.5 precisely to probe this, but with k_TupleCount = 5 the array only holds 0…4, so the one divergent element (value 5) never exists.

Confirmed by bumping k_TupleCount to 8 — Valid Single Thresholds: Int then fails for <, ==, and != at threshold 5.5, e.g. 5 == 5.5 is true for the filter (cast to 5) and false for the oracle.

Legacy performs the identical cast (SIMPL/Source/SIMPLib/Filtering/ThresholdFilterHelper.h:60), so this line is correct and the oracle helper is what's wrong. Please model the truncation in the Expected*Mask helpers and record comparison-value truncation as verified, legacy-matching behavior in the report's Oracle section — otherwise any later increase in fixture size will present as a regression.