DO NOT MERGE - AI prototype of reduction algorithm framework

[blowekamp]

Prototype of a generic ReduceAlgorithm interface (Greedy, Tree, Linear
strategies) for combining per-work-unit partial results, plus a
refactor of LabelStatisticsImageFilter, LabelOverlapMeasuresImageFilter,
and ImageToHistogramFilter to use GreedyReduceAlgorithm instead of
hand-rolled mutex swap-and-merge loops. Opened as a draft for discussion,
not intended to merge as-is.

Background

Follows up on the deterministic-parallel-reduce discussion in #6622
(Mattes metric derivatives). GreedyReduceAlgorithm matches the
existing non-deterministic swap-and-merge pattern; TreeReduceAlgorithm
and LinearReduceAlgorithm offer deterministic, chunk-ID-ordered
merging for cases that need reproducible floating-point results.

AI assistance

• Tool: GitHub Copilot (Claude Sonnet 4.5)
• Role: implemented the ReduceAlgorithm class hierarchy and GTest
  suites, and refactored the three filters to use GreedyReduceAlgorithm
• All code was built and tested locally before committing

Test results

• ITKCommonGTestDriver (Greedy/Tree/Linear reduce tests): all passing
• ITKImageStatisticsTestDriver/GTestDriver, ITKStatisticsTestDriver/GTestDriver: all passing after fetching ExternalData

[hjmjohnson]

IMHO: Given the scope of what is needed here, and the impact, it seems like a good candidate to schedule as part of a funded effort where adequate team resources can be assigned and committed to. Or at least given a separate planning session.

It feels like this is the tip-of the iceberg of "work that should be done".

==========================

Ran a "/code-review max" over the PR prototype — the TreeReduceAlgorithm merge is the strong part here, and it traced its phantom-leaf padding for N = 3, 5, 6, 7, 9, 15, and the ordering holds. Three things flagged before this goes further, the first being purely mechanical:

1. Two of the three new GTest files are never built. Modules/Core/Common/test/CMakeLists.txt:1482 adds only itkLinearReduceAlgorithmGTest.cxx; itkGreedyReduceAlgorithmGTest.cxx and itkTreeReduceAlgorithmGTest.cxx (28 TESTs, 749 lines) aren't in ITKCommonGTests. So GreedyReduceAlgorithm — the only class actually wired into the three filters — currently has no coverage at all.

2. No chunk-ID plumbing exists in ITK's threading API, which I think is the central design question rather than a defect. ThreadedStreamedGenerateData(const RegionType &) exposes no work-unit index, and MultiThreaderBase may run fewer work units than requested. Tree and Linear both require exact 0..N-1 coverage, so neither is adoptable by a filter today — which is presumably why the PR wires in Greedy everywhere. That does mean the three refactors are determinism-neutral by construction.

3. Three silent-wrong-answer paths where a wrong result is returned with no exception — details below.

The three silent-failure paths

• LinearReduceAlgorithm::GetResult() is destructive and latches. It consumes m_Values (value.reset()) and sets m_Merged = true; every later call short-circuits on if (!m_Merged). Any call before all chunks have deposited permanently discards the remaining chunks. It also mutates through a const API.
• Duplicate chunkId corrupts the tree. A repeated chunk overwrites the leaf and double-increments the parent counter. The overshoot propagates: a later real sibling sees prev == 2 and re-merges children that were already reset(), so the node becomes nullopt and that subtree's accumulated value is dropped. The root can end up nullopt, and GetResult() then returns the default-constructed m_DefaultResult.
• A throwing merge functor in Greedy destroys both operands. m_MergeFunction(localResult, tomerge) runs after the lock is released with m_HasResult == false and the accumulator already swapped out. A throw — or an empty std::function, which nothing validates — loses localResult and the entire accumulated tomerge. Other threads then deposit onto an empty accumulator and the reduction completes "successfully" with data missing.

Related: a missing chunk is also silent (Tree returns m_DefaultResult, Linear returns a partial sum). Some kind of completeness check or Finalize() would turn all of these into loud failures.

Thread-safety and lifecycle

• LinearReduceAlgorithm::GetResult() can self-deadlock — it holds the non-recursive m_Mutex across every m_MergeFunction call. Greedy deliberately calls the functor outside the lock; Linear inverts that.
• TreeReduceAlgorithm::SetNumberOfWorkUnits() / BuildTree() / Clear() take no lock and are racy against an in-flight Merge(): m_Values.assign() reallocates and m_ChildrenReady is replaced wholesale.
• No release/acquire between the final root merge and GetResult() in Tree — formally a race. For N == 1 the leaf is the root (m_PaddedSize + 0 == 1), so the value is published with no synchronization at all. Benign behind a pool join, but the class ships as general-purpose Core/Common API and its own GTest drives it with bare std::thread.
• Clear() means two different things in one hierarchy. Greedy calls Superclass::Clear(), which zeroes m_NumberOfWorkUnits; Tree and Linear deliberately don't, and document "Does not change ... work-unit count." A caller holding a ReduceAlgorithm<T> * can't know whether reuse-after-Clear() works.
• Tree and Linear produce different deterministic orders (balanced tree vs. left fold), so the two "deterministic" backends aren't interchangeable for floating point — worth stating in the docs.

Filter refactor: a deep copy replaces a swap

itkLabelStatisticsImageFilter.hxx and itkLabelOverlapMeasuresImageFilter.hxx both now do:

m_LabelStatistics = m_Reducer->GetResult();

GetResult() returns const T &, so this deep-copies the whole label map including the per-label Histogram smart pointers, where the previous code swap-ed. A move-out or swap accessor on the reducer would avoid it.

Minor: std::mutex m_Mutex{} in itkImageToHistogramFilter.h is now dead (its only user was the removed merge loop) but is still a protected member. PrintSelf isn't updated for m_Reducer in any of the three filters.

Graft(m_Reducer->GetResult()) followed by Clear() looked like a lifetime hazard, but the base commit does the same thing (Graft(m_MergeHistogram); m_MergeHistogram = nullptr;) — pre-existing, not introduced here.

Why the tests wouldn't catch the above

The concurrency tests are structurally sound — real std::thread fan-out, join, then assert. But:

• Every Tree/Linear ordering test (NonPowerOfTwo_*, ChunksInOrder / Reversed / OutOfOrder) reduces with integer addition, which is associative and commutative — so a wrong tree grouping or a reversed merge direction is undetectable.
• DeterministicFloat is the only order-sensitive test, and it uses N = 8, a power of two. The padded/phantom path — the only genuinely non-trivial ordering logic — has zero floating-point coverage.
• Nothing covers: duplicate chunkId, a missing chunk, GetResult() before all merges, an unset merge function, a throwing functor, or Merge() after GetResult() / Clear(). GetResultBeforeMerge actually triggers the Linear latching bug without noticing.

Smaller notes

• GreedyReduceAlgorithm::Merge() invokes m_MergeFunction with no validation; Linear and Tree guard their preconditions with itkExceptionMacro, Greedy guards nothing.
• The merge functors in all three filter constructors capture a raw this, coupling reducer lifetime to filter lifetime. Fine today only because MergeMap is const.
• Comment nits: the two-line "...deterministically ordered is not required here." in itkLabelStatisticsImageFilter.hxx reads as ungrammatical, and the 6-line block in itkGreedyReduceAlgorithm.hxx restates the header Doxygen.

Reviewed against the reduce-interface request in #6622. Happy to open a PR against the branch for the CMake registration if that's useful.

_{Review assisted by Claude Code (Opus 5); findings were traced against the source and verified by hand before posting.}