BUG: Make N4BiasFieldCorrectionImageFilter work with GlobalReleaseDataFlag by gdevenyi · Pull Request #7093 · InsightSoftwareConsortium/ITK

gdevenyi · 2026-11-02T22:56:10Z

Make N4BiasFieldCorrectionImageFilter run with DataObject::GlobalReleaseDataFlag on; on main it throws "outside of buffered region" when the control point lattice is read after a reconstruction. The output is unchanged.

Root cause

N4 passes the control point lattice, which it reuses later, as an input of internal filters. After each internal update, ProcessObject::ReleaseInputs() calls ReleaseData() on those inputs when ShouldIReleaseData() is true, which is always the case with the global flag. The lattice is the input of the B-spline reconstructers (through MakeBiasFieldReconstructer, for every iteration's field, the final field and ReconstructBiasField), and is read again by the adder, by RefineControlPointLattice and by callers of GetLogBiasFieldControlPointLattice.

Before #7072, #7075 and #7085 merged, the log input, the sharpened image and the bias field estimates went through internal subtract filters too; main now computes those differences in place, so only the lattice is left.

Found while answering a review on #7075.

Fix

A private helper, MakeInputView(image), returns a new image that grafts the given one, so it shares the buffer. MakeBiasFieldReconstructer, which every reconstruction uses, passes such a view instead of the lattice itself. A release then initializes only the view, and the original keeps its buffer. Before RefineControlPointLattice, the reconstructer gets a fresh view, because the refinement reads its input after the update. No values or summation orders change.

Test results

New GTest N4BiasFieldCorrectionImageFilter.GlobalReleaseDataFlagDoesNotChangeOutput: N4 on a 40 x 40 image with a bias field, a mask and a graded confidence image (2 levels x 4 iterations), normally and under a guard that turns the global flag on and restores it, compared with EXPECT_EQ. It throws on main and passes with this PR.
Rebased onto main with PERF: Build N4 B-spline fit points once instead of in each iteration #7075 and PERF: Reuse N4 iteration buffers to cut per-iteration allocations #7085: ctest -R 'BSpline|N4|BiasCorrection' passes 42/42, and the build adds no warnings. Without the view in MakeBiasFieldReconstructer, the new GTest throws.
A local harness (MNI152 T1, four N4 runs: no mask, mask, mask and confidence image, oblique direction) gives byte-identical output before and after this change.

AI assistance

Tool: Claude Code (claude-opus-5-5)
Role: found the bug while addressing a review on PERF: Build N4 B-spline fit points once instead of in each iteration #7075, traced the released inputs, wrote the fix and the GTest, and ran the local build, tests and bit-identity check
All code was reviewed and tested locally before committing

gdevenyi · 2026-11-02T22:56:18Z

@ntustison could you take a look when you have a moment? This came out of the N4 review on #7075.

hjmjohnson · 2026-11-03T01:49:00Z

@greptileai review

greptile-apps · 2026-11-03T01:53:07Z

Confidence Score: 4/5

[Medium impact] The change affects N4 bias correction when global data release is enabled.

The buffer changes appear sound, but the existing image-iteration requirement must be satisfied before merging.

Findings

Output copy skips image iterators ▶

Summary

This PR keeps N4's reusable images alive when GlobalReleaseDataFlag is enabled.

N4 keeps its working images through internal filter updates.

_{Reviews (2) · Last reviewed commit: "BUG: Make N4BiasFieldCorrectionImageFilt..." · Reviewed by Greptile}

greptile-apps · 2026-11-03T01:53:11Z

+  filter->Update();
+
+  const N4ReleaseImageType * output = filter->GetOutput();
+  return { output->GetBufferPointer(), output->GetBufferPointer() + output->GetBufferedRegion().GetNumberOfPixels() };


Output copy skips image iterators

The new output copy builds a vector from raw buffer pointers. The project's image iteration standard says, “Use iterators, not raw buffer access.” Build the returned vector from image iterators to satisfy this repository requirement before merging.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

hjmjohnson

Thanks. The root-cause list checks out: every input that is reused after a release is now passed as a view, the fresh view before RefineControlPointLattice is there, and the test fails on main (reasoned from the code: subtracter2 reads the released newLogBiasField). The test throws in the first iteration's subtracter2, not "at the second iteration" as the body says.

Requesting changes for one item, which is Greptile's P1 as well: the view in the public ReconstructBiasField changes its behavior for callers outside the filter (inline).

Non-blocking:

MakeInputView is a template defined in the .h; the module layout puts template bodies in the .hxx.
The test compares a flagged run with an unflagged one, with UseMaskLabel at its default. The fixed-baseline release-flag test #7075 deferred to this PR can follow once both merge.
The commit body's sentences "The output is unchanged. A GoogleTest runs N4 ..." restate the diff.
The PR body still ends with the "Generated with Claude Code" footer; the AI disclosure in <details> is enough (non-blocking).

This review ran as /code-review max (Standards and Spec axes) on Opus 5.5. Once these are addressed I will re-check, then request a Greptile review and mark the PR ready.

hjmjohnson · 2026-11-03T03:08:00Z

  using BSplineReconstructerType = BSplineControlPointImageFilter<BiasFieldControlPointLatticeType, ScalarImageType>;
  auto reconstructer = BSplineReconstructerType::New();
-  reconstructer->SetInput(controlPointLattice);
+  reconstructer->SetInput(MakeInputView(controlPointLattice));


Should-fix: ReconstructBiasField is public. Wrapping the caller's lattice in a graft-based view here means a lattice that is an un-updated pipeline output (a reader's GetOutput() before Update()) is no longer brought up to date: the view has no source, and Graft copies regions and the pixel container but not the producer. That is a silent behavior change on a public method. The only in-tree caller is line 607, so the view can move there and leave the public method as on main:

Suggested change

reconstructer->SetInput(MakeInputView(controlPointLattice));

reconstructer->SetInput(controlPointLattice);

and at line 607: this->ReconstructBiasField(MakeInputView(this->m_LogBiasFieldControlPointLattice.GetPointer())).

Done in 4a1014a, as suggested: the public ReconstructBiasField sets the caller's lattice as its input again, exactly as on main, and the only in-tree caller passes MakeInputView(this->m_LogBiasFieldControlPointLattice.GetPointer()). Removing that call-site view makes GlobalReleaseDataFlagDoesNotChangeOutput fail, so the test still covers it.

gdevenyi · 2026-11-03T03:26:12Z

Thanks @hjmjohnson. Force-pushed 4a1014a. Besides the inline item: MakeInputView is declared in the .h and defined in the .hxx; the commit message no longer restates the diff; and the PR summary now says it throws in the first iteration's second subtraction. The fixed-baseline release-flag test from #7075 can follow once both are in, as you suggest. ctest -R 'N4|BiasCorrection' passes 9/9.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

…aFlag N4 passes the control point lattice, which it reuses later and returns from GetLogBiasFieldControlPointLattice, as the input of its internal B-spline reconstruction and refinement. With DataObject::GlobalReleaseDataFlag on, those filters released it after their update, and the next use threw "outside of buffered region". Pass a view that grafts the lattice instead, so a release frees only the view. MakeBiasFieldReconstructer, which every reconstruction uses, makes the view, and the refinement gets a fresh one.

gdevenyi · 2026-11-04T17:40:30Z

Rebased onto main with #7075 and #7085 merged (32fc255). Those PRs replaced N4's internal subtract filters with in-place loops, so this PR's views on the log input, the sharpened image and the field estimates are gone with them. The control point lattice still goes through the B-spline reconstructers. That now happens in one place, MakeBiasFieldReconstructer, which passes it a view. Without that view, the GlobalReleaseDataFlag GTest throws. ctest -R "BSpline|N4|BiasCorrection" passes 42/42. The description is updated to match.

This was referenced Nov 2, 2026

PERF: Coordinate ITK upstream performance fixes (N4 B-spline fit, N4 point set, AdaptiveDenoising box mean and thread race) ANTsX/ANTs#2037

Open

PERF: Build N4 B-spline fit points once instead of in each iteration #7075

Merged

greptile-apps Bot reviewed Nov 3, 2026

View reviewed changes

hjmjohnson requested changes Nov 3, 2026

View reviewed changes

gdevenyi force-pushed the bug-n4-global-release-data-flag branch from 1d7d6f3 to 4a1014a Compare November 3, 2026 03:25

gdevenyi marked this pull request as ready for review November 3, 2026 10:09

Copilot AI balanced review requested due to automatic review settings November 3, 2026 10:09

Copilot AI reviewed Nov 3, 2026

gdevenyi force-pushed the bug-n4-global-release-data-flag branch from 4a1014a to 38dc67e Compare November 3, 2026 16:49

gdevenyi force-pushed the bug-n4-global-release-data-flag branch from 38dc67e to 32fc255 Compare November 4, 2026 17:40

Navigation Menu

Repository navigation

BUG: Make N4BiasFieldCorrectionImageFilter work with GlobalReleaseDataFlag - #7093

BUG: Make N4BiasFieldCorrectionImageFilter work with GlobalReleaseDataFlag#7093
gdevenyi wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
gdevenyi:bug-n4-global-release-data-flag

gdevenyi commented Nov 2, 2026 •

edited

Loading

Uh oh!

gdevenyi commented Nov 2, 2026

Uh oh!

hjmjohnson commented Nov 3, 2026

Uh oh!

greptile-apps Bot commented Nov 3, 2026 •

edited

Loading

Summary

Uh oh!

Uh oh!

greptile-apps Bot Nov 3, 2026

Uh oh!

hjmjohnson left a comment

Uh oh!

hjmjohnson Nov 3, 2026

Uh oh!

gdevenyi Nov 3, 2026

Uh oh!

gdevenyi commented Nov 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

gdevenyi commented Nov 4, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	reconstructer->SetInput(MakeInputView(controlPointLattice));
	reconstructer->SetInput(controlPointLattice);

Conversation

gdevenyi commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdevenyi commented Nov 2, 2026

Uh oh!

hjmjohnson commented Nov 3, 2026

Uh oh!

greptile-apps Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

Uh oh!

greptile-apps Bot Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

hjmjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

hjmjohnson Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

gdevenyi Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

gdevenyi commented Nov 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

gdevenyi commented Nov 4, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gdevenyi commented Nov 2, 2026 •

edited

Loading

greptile-apps Bot commented Nov 3, 2026 •

edited

Loading