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.

[thomas-albrecht]

Here's an AI description why the code does the same thing:

The old code uses two separate loops over k:

First loop: count into the float matrix
Second loop: normalize each row by its sum
The new code merges both steps into one loop over k: for each input k, it counts into the integer matrix, then immediately normalizes that input's confusion matrix before moving to the next k.

This is valid because there's no cross-dependency between inputs — the confusion matrix for input k is populated and normalized entirely independently of any other input k'. The old code's separation into two loops was unnecessary; it just happened to be written that way.

[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.