Skip to content

BUG: Backport zero-check guard in CumulativeGaussianCostFunction#6067

Open
hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-cumulative-gaussian
Open

BUG: Backport zero-check guard in CumulativeGaussianCostFunction#6067
hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-cumulative-gaussian

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Adapted from d7845fe on main. Adds numberOfElements == 0 check before the size-mismatch check in CalculateFitError to guard against division-by-zero.

On main this was a reorder of an existing compound condition; on release-5.4 the zero check was entirely absent.

Backport for #6051 (Tier 1).

Guard the division by numberOfElements on line 55 against
division-by-zero when m_OriginalDataArray is empty. The existing
size-mismatch check did not catch the zero case.

Cherry-pick of d7845fe from main, adapted for release-5.4
where the zero check was entirely absent (main had it in the
wrong order).
@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:Numerics Issues affecting the Numerics module labels Apr 15, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 15, 2026

Greptile Summary

This backport adds a numberOfElements == 0 early-return guard in CalculateFitError (line 46) via short-circuit || evaluation, preventing a division-by-zero crash that was entirely absent in the release-5.4 branch. The fix is correctly ordered and the guard logic is sound.

Confidence Score: 5/5

Safe to merge; the zero-check guard is correct and no new defects are introduced.

The primary fix is correct — numberOfElements == 0 is checked before the mismatch check using short-circuit evaluation, fully preventing division-by-zero. The only finding is a pre-existing integer division issue (1 / numberOfElements) that is unrelated to this PR and does not block merge.

No files require special attention.

Important Files Changed

Filename Overview
Modules/Numerics/Optimizers/src/itkCumulativeGaussianCostFunction.cxx Adds numberOfElements == 0 guard via short-circuit `

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["CalculateFitError(setTestArray)"] --> B["numberOfElements = m_OriginalDataArray.GetNumberOfElements()"]
    B --> C{"numberOfElements == 0?"}
    C -- "Yes (NEW guard)" --> D["return 1"]
    C -- "No" --> E{"numberOfElements != setTestArray->size?"}
    E -- "Yes" --> D
    E -- "No" --> F["Sum squared differences in loop"]
    F --> G["return sqrt((1/numberOfElements) * fitError)"]
    style C fill:#90ee90,stroke:#228B22
    style D fill:#ffcccc
Loading

Comments Outside Diff (1)

  1. Modules/Numerics/Optimizers/src/itkCumulativeGaussianCostFunction.cxx, line 55 (link)

    P2 Pre-existing integer division yields incorrect RMSE

    1 / numberOfElements performs integer division because both operands are integer types (int literal and unsigned int). For any numberOfElements > 1, this evaluates to 0, making sqrt(0 * fitError) == 0.0 — the RMSE is silently wrong for all non-trivial arrays. The PR correctly guards against numberOfElements == 0, but since this function is being modified, this is a good opportunity to fix the formula.

Reviews (1): Last reviewed commit: "BUG: Add zero-check guard in CumulativeG..." | Re-trigger Greptile

Copy link
Copy Markdown
Member Author

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

greptile comments need to ensure that upstream/main has already fixed this, if not fix upstream/main and PR.

This is a backport from main -> release-5.4.

Copy link
Copy Markdown
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hjmjohnson thanks!

@thewtex thewtex added this to the ITK 5.4.6 milestone Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Numerics Issues affecting the Numerics module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants