BUG: Use label integers instead of float to count pixels

[thomas-albrecht]

The original count uses ++ on a WeightsType, which defaults to float, to count pixels.
When there are more than ~ 16.7 million pixels to be counted, which happens quickly in 3D images (times number of inputs), the ++ has no more effect as 16,777,217 has the same float representation as 16,777,216.

The fix is simple: Do the counting in integers (size_t) and then the final operation

label_frequency = label_count / total_count

in float (WeightsType).

PR Checklist

• 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 Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

[github-actions]

Thank you for contributing a pull request! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.

[greptile-apps]

Greptile Summary

This change replaces floating-point label counters with integer counters before calculating automatic prior probabilities. It also leaves the final element of the returned automatic-prior array unwritten: the array has one more element than the assignment loop covers. Restore initialization of the full array after allocation so callers always receive a defined trailing probability.

A focused C++ regression ran the automatic-prior path with default floating-point weights and read the terminal returned entry. The allocation happened to contain zero in the observed run, but the implementation has no write for that returned slot and no longer retains the parent implementation's full-array initialization.

Confidence Score: 4/5

This change should not merge until the complete automatic-prior array is initialized.

The changed bounds and the removed full-array initialization directly establish that one publicly returned element is unwritten. The focused native regression exercised that automatic-prior path; its observed zero terminal value was allocator-dependent and does not establish initialization.

Files Needing Attention: Modules/Segmentation/LabelVoting/include/itkMultiLabelSTAPLEImageFilter.hxx needs the full prior-probability array initialized after SetSize.

T-Rex Logs

What T-Rex did

• T-Rex produced a finding-comment proof for a posted P1 finding and attached a C++ regression source and two regression-output artifacts.
• T-Rex produced a second finding-comment-proof for another P1 finding.
• T-Rex produced a general-contract-validation-proof that shows the pre- and post-output state of the trailing prior (prior_size=4, prior_values=0.333333343,0.333333343,0.333333343,0; trailing_index=3 trailing_value=0 isfinite=1) and confirms the trailing automatic prior is initialized to zero with exit-code 0.

_{Ran code and verified through T-Rex}

Comments Outside Diff (1)

1. General comment

   Automatic-prior array leaves its final allocated element uninitialized

   • Bug
     • In the automatic-prior path, the filter returns an array of 1 + totalLabelCount values but writes only indices 0 through totalLabelCount - 1. The final returned prior can therefore contain an indeterminate float value. The narrow execution reached this path and read the trailing entry, but this allocator happened to return zero in both parent and PR-head runs.
   • Cause
     • PR BUG: Use label integers instead of float to count pixels #6759 replaced the previous float counting implementation, which allocated the array and called m_PriorProbabilities.Fill(0.0), with an integer count vector and omitted the equivalent initialization after m_PriorProbabilities.SetSize(...).
   • Fix
     • Initialize the complete prior array immediately after SetSize, for example m_PriorProbabilities.Fill(0.0), before assigning the computed label priors; retain a regression assertion for the trailing entry.

   _{Ran code and verified through T-Rex}

_{Reviews (1): Last reviewed commit: "Use label integers instead of float to c..." | Re-trigger Greptile}

[dzenanz]

Looks good to me.

[dzenanz]

TODOs: merge into one commit, add commit message prefix as per ghostflow. Preferably before merging, latest upon merge.

[blowekamp]

Thank you for identifying the issue and contributing. I started to look into this and then saw you PR here.

Here are some additional AI recommendation I found:

The PR correctly fixes InitializePriorProbabilities() but leaves the same float-counting bug unfixed in InitializeConfusionMatrixArrayFromVoting():

This is arguably the more impactful site because these counts initialize the confusion matrices used throughout all EM iterations. A consistent fix using a local std::vector<size_t> (or Array2D) for counting, followed by normalization into TWeights, should be applied there as well, mirroring the pattern used in InitializePriorProbabilities().

Additional observations:

EM M-step accumulation unaddressed — m_UpdatedConfusionMatrixArray[k][j][ci] += W[ci] still accumulates TWeights (float) sums over all pixels. For large 3D images this is a secondary but real precision issue; using double internally for m_UpdatedConfusionMatrixArray (which is private, so no API impact) would resolve it.

ITK style nit — std::vector<size_t> is fine, but SizeValueType (ITK's canonical pixel-count type, defined as uint64_t) would be more idiomatic.

[thomas-albrecht]

Cool, thanks for the feedback. Of course initially I wanted to be as surgical as possible and only change the things that directly affected the problem I found instead of giving the class a general overhaul.

But all your points are valid, am I "allowed" to go ahead and make the changes myself?

[dzenanz]

That can also be a follow-up PR.

[thomas-albrecht]

I made the changes and pushed them already.
There were anyway some checks still pending or failing.

[dzenanz]

I made through review earlier, and a glancing review after the scope increase. Everything looked OK. Is this functionality being exercised by any tests?

[thomas-albrecht]

It looks to me like there are extensive tests in
https://github.com/InsightSoftwareConsortium/ITK/blob/2f6c5acceda891da57371a2a82542c8f8663ca67/Modules/Segmentation/LabelVoting/test/itkMultiLabelSTAPLEImageFilterTest.cxx
that should have failed in the github actions if anything got broken.

[hjmjohnson]

The Greptile P1 ("Automatic-prior array leaves its final allocated element uninitialized") is a false positive — Fill(0.0) is still there, immediately after SetSize:

this->m_PriorProbabilities.SetSize(1 + static_cast<SizeValueType>(totalLabelCount));
this->m_PriorProbabilities.Fill(0.0);   // <-- retained
for (SizeValueType l = 0; l < totalLabelCount; ++l)
{
  this->m_PriorProbabilities[l] = static_cast<WeightsType>(labelCounts[l]) / static_cast<WeightsType>(totalCount);
}

The trailing entry is deterministically zero, not indeterminate. Greptile's own T-Rex run observed trailing_value=0 and attributed it to the allocator; it is in fact the Fill.

I also audited the head commit for any remaining integer-count-in-float accumulators. There are none — all three items from @blowekamp's review are addressed, and every surviving floating-point accumulator is genuinely fractional.

Audit detail

Review item	Status
InitializeConfusionMatrixArrayFromVoting() counting in float	Fixed — Array2D<SizeValueType> counts, normalized into WeightsType
M-step m_UpdatedConfusionMatrixArray += W[ci] in float	Fixed — member retyped to std::vector<Array2D<double>>
size_t → SizeValueType idiom	Applied in both functions

Remaining floating-point accumulators, all correct as-is:

• W[ci] *= ... / sumW += W[ci] — per-pixel posterior weights, at most m_TotalLabelCount terms; never a pixel count.
• m_UpdatedConfusionMatrixArray[k][j][ci] += W[ci] — the one accumulation that does grow with pixel count, now in double (saturates at 2^53, unreachable).
• sumW in the column normalization — double after this PR.
• maximumUpdate — a max, not a sum.

One note for the commit message: m_ConfusionMatrixArray (Array2D<TWeights>) and m_UpdatedConfusionMatrixArray (Array2D<double>) now differ in type, so the copy-back narrows double → float each iteration. Harmless for values in [0,1], and API-safe since GetConfusionMatrix() still returns ConfusionMatrixType — but a sentence explaining the asymmetry would preempt the question.

Separately and out of scope here: itkSTAPLEImageFilter.hxx (binary STAPLE) counts pixels with N = N + 1.0. It is double, so it does not actually break, but it is the same idiom if anyone wants a follow-up cleanup. itkLabelVotingImageFilter already counts votes in unsigned int.