PERF: Build N4 B-spline fit points once instead of in each iteration by gdevenyi · Pull Request #7075 · InsightSoftwareConsortium/ITK

gdevenyi · 2026-11-02T17:35:02Z

Build the N4 B-spline fit points and weights once per GenerateData, and in each iteration only gather the residual field values into the point data, in parallel. The output is bit-identical; on main (which now includes #7072), full-resolution N4 is 20 % faster.

Details

The points and weights depend only on the mask, the confidence image and the input geometry. Each iteration rebuilt them in a serial loop with a mask test and TransformIndexToPhysicalPoint per voxel (ANTsX/ANTs#2037, item N4-3). The points are built from an image with the input origin, spacing and buffered region and identity direction, as the importer did, so their coordinates are unchanged, also on oblique images. The members are released at the end of GenerateData.

Cost: the points, the weights and a SizeValueType buffer index per included voxel now stay allocated for the whole run, not for one iteration at a time.

Measurements

AMD Ryzen 9 9950X, GCC 13.3, Release, 16 threads. N4 on MNI152 2009c T1 (1 mm, no shrink, 193 x 229 x 193), 3 levels x 15 iterations, four runs (no mask; mask; mask and confidence image; oblique direction), two repeats:

	wall	peak RSS
`main` with #7072	58.2 s	600 MB
`main` with #7072 + this PR	46.4 s	690 MB

At shrink 2 on main before #7072 merged, the gain was within noise (43.8 s → 43.3 s), because the fit dominated there.

Test results

ctest -R 'N4|BiasCorrection': 9/9 pass. The build adds no warnings.
Bit-identity: the raw output and control-point lattice of the four N4 runs above at shrink 2, plus a re-Update() after removing the mask, with cmp against main: identical, for this PR alone and stacked on PERF: Compute B-spline fit kernel values once per axis per point #7072.

AI assistance

Tool: Claude Code (claude-opus-5-5)
Role: implemented the change from the analysis in PERF: Coordinate ITK upstream performance fixes (N4 B-spline fit, N4 point set, AdaptiveDenoising box mean and thread race) ANTsX/ANTs#2037, wrote the bit-identity harness, and ran the local build, tests and timings
All code was reviewed and tested locally before committing

gdevenyi · 2026-11-02T18:03:16Z

@ntustison could you review this when you have a chance? It is one of the upstream items from ANTsX/ANTs#2037.

hjmjohnson

Thanks @gdevenyi. Hoisting the point construction out of the iteration loop is worth doing, but the cached PointSet does not survive the release-data path, and the change needs an in-tree test before merge.

Please add a GoogleTest that runs N4 with UseMaskLabel on and a mask label set, a confidence image, and a second run with DataObject::GlobalReleaseDataFlag on, and compares the result against a fixed baseline. No current N4 test sets a mask label or a confidence image, and the bit-identity check is out of tree, so the equivalence should be pinned by an in-tree test.

Other details are inline.

gdevenyi · 2026-11-02T21:41:23Z

Thanks @hjmjohnson. I force-pushed 76f7e68 with the inline changes. Three new GoogleTests are in itkN4BiasFieldCorrectionImageFilterGTest.cxx, all on a 2D image with a bias field and a three-label mask:

MaskLabelMatchesBinaryMask: UseMaskLabel with label 1 gives exactly the output of the equivalent binary mask.
BinaryConfidenceMatchesMask: a {0, 1} confidence image gives exactly the output of the equivalent label mask.
MaskLabelAndConfidenceMatchBaseline: a mask label together with a graded confidence image (0 in one band) gives the same output as main at 5 voxels (values generated with main's N4, relative tolerance 1e-4), and a second run is exactly equal. It fails if the confidence weights are ignored.

All three pass with main's N4 and with this PR. The out-of-tree N4 harness is also byte-identical to main.

I couldn't add the GlobalReleaseDataFlag run. N4 already throws with that flag on main, independent of this PR: at the second iteration, the internal subtract filters have released logInputImage and logUncorrectedImage (and the adder releases the control-point lattice), so an iterator on the released buffer throws "outside of buffered region". With this revision the fit's point set is no longer one of those cases. Making all of N4 safe under the flag would need its intermediates kept out of mini-pipeline inputs. That's a separate change, and I can open an issue or PR for it if you'd like.

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

Follow-up to the GlobalReleaseDataFlag point in my summary above: the pre-existing N4 failure under that flag now has its own PR, #7093. Once both merge, I can add the release-flag run to the tests here.

Round-1 requested changes are addressed; dismissing my change request.

greptile-apps · 2026-11-03T01:31:26Z

Confidence Score: 4/5

[Medium impact] The change affects runtime and memory use within N4 bias correction.

The remaining raw-buffer copy in the test must meet the repository’s image-access requirement before merging.

Findings

Raw reads bypass image conventions ▶

Summary

N4 builds fit points and weights once per GenerateData call.

N4 reuses B-spline fit points and weights throughout each run.

_{Reviews (2) · Last reviewed commit: "PERF: Build N4 B-spline fit points once ..." · Reviewed by Greptile}

greptile-apps · 2026-11-03T01:31:29Z

-
+  auto &                 values = fitPoints.values->CastToSTLContainer();
+  const auto &           bufferIndices = fitPoints.bufferIndices;
+  const RealType * const fieldBuffer = fieldEstimate->GetBufferPointer();


Raw reads bypass image conventions

The new gather reads pixels through GetBufferPointer(), which violates the project directive “Use iterators, not raw buffer access.” Use MakeImageBufferRange(fieldEstimate) for the indexed reads instead. The new output copy in itkN4BiasFieldCorrectionImageFilterGTest.cxx also needs range-based access. This repository requirement must be satisfied 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!

Fixed in b8322a4: the gather now reads through MakeImageBufferRange(fieldEstimate), the same way GenerateData reads the mask and confidence images.

Copilot

🟢 Approval recommended

No active review issues are recorded for the N4 bias-correction optimization.

No open issues 🎉

What changed in this PR

This PR reduces repeated work in N4 bias correction by building B-spline fit points and weights once per filter execution, then gathering only residual values in parallel during each iteration. It complements the B-spline fitting optimization in #7072.

Changes:

Reuse fit coordinates, weights, and voxel buffer indices throughout GenerateData.
Parallelize residual-value gathering while creating a fresh point set for each fit.
Add regression tests for mask labels, confidence images, and baseline output values.

File	Description
Modules/Filtering/BiasCorrection/test/itkN4BiasFieldCorrectionImageFilterGTest.cxx	Adds mask/confidence equivalence and baseline-output tests.
Modules/Filtering/BiasCorrection/include/itkN4BiasFieldCorrectionImageFilter.hxx	Builds reusable fit data once and gathers residual values in parallel.
Modules/Filtering/BiasCorrection/include/itkN4BiasFieldCorrectionImageFilter.h	Defines `FitPoints` and updates the bias-estimation method signature.

_{Review effort: Balanced}

hjmjohnson

The new head is a rebase of the round-2 code (git range-diff shows the patch unchanged), so the spec side is still clean: bit-identity holds by reading, and every round-1 thread checks out. Three should-fix items remain on the standards side, inline. None is blocking.

Also, Greptile's open P2 on the raw GetBufferPointer() gather (hxx:517) holds: Documentation/AI/conventions.md says "Use iterators, not raw buffer access", and GenerateData already uses MakeImageBufferRange for the mask and confidence images. const auto fieldRange = MakeImageBufferRange(fieldEstimate); with fieldRange[bufferIndices[k]] is a drop-in. Please fix it or answer that thread.

Non-blocking:

"with #7072" in the body is out of date now that #7072 has merged.
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:07:42Z

   * the estimate and adds the resulting control point values to the total
   * bias field estimate.
   */
+  /** The fit points of the included voxels, which depend only on the mask, the confidence image and the geometry. */


Should-fix: FitPoints sits between UpdateBiasFieldEstimate's doc block (lines 409-413) and the method, so Doxygen attaches that block to the struct and the method is left undocumented. The struct comment is also the commit-body sentence round 1 asked to keep out of the source. Please move the struct above the doc block at line 409, so the doc block sits directly on the method, with a short comment:

Suggested change

/** The fit points of the included voxels, which depend only on the mask, the confidence image and the geometry. */

/** Fit points of the included voxels. */

Done in b8322a4: FitPoints now sits above the doc block, with your one-line comment, so the doc block attaches to UpdateBiasFieldEstimate.

hjmjohnson · 2026-11-03T03:07:42Z

+  const auto confidence = MakeConfidence();
+  const auto output = RunN4({ labelMask, true, 1, confidence });
+
+  // Values from ITK main before the fit points were built once per filter update.


Should-fix: comments describe what the code is, not what it replaced (prose-budget.md, references to prior behavior):

Suggested change

// Values from ITK main before the fit points were built once per filter update.

// Regression baseline.

Done, using your suggestion.

hjmjohnson · 2026-11-03T03:07:42Z

-
-  const typename ImporterType::OutputImageType * parametricFieldEstimate = importer->GetOutput();
-
+  auto &                 values = fitPoints.values->CastToSTLContainer();


Nit: this writes through const FitPoints &; it compiles only because the SmartPointer is const and its target is not. Taking FitPoints & would make the signature say the values are filled here.

Done: UpdateBiasFieldEstimate now takes FitPoints &.

gdevenyi · 2026-11-03T03:29:24Z

Thanks @hjmjohnson. Force-pushed b8322a4 with the three items and greptile's buffer-range point (all inline). The PR body no longer refers to #7072 as pending. ctest -R 'N4|BiasCorrection' passes 11/11. #7085 is rebased onto it (7af9818), passing 38/38.

The fit points and weights depend only on the mask, the confidence image and the input geometry. Build their containers once in GenerateData, and in each iteration gather only the residual field values, in parallel, into a fresh point set that shares the containers, so releasing the fit's input cannot empty them. The points, weights and values are unchanged, so the output is bit-identical. GoogleTests compare a mask label with a binary mask, a binary confidence image with a mask, and a mask label with a confidence image against values from main.

gdevenyi mentioned this pull request 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

github-actions Bot added type:Performance Improvement in terms of compilation or execution time area:Filtering Issues affecting the Filtering module labels Nov 2, 2026

gdevenyi mentioned this pull request Nov 2, 2026

PERF: Reuse N4 iteration buffers to cut per-iteration allocations #7085

Merged

hjmjohnson previously requested changes Nov 2, 2026

View reviewed changes

gdevenyi force-pushed the perf-n4-persistent-point-set branch from 7529e85 to 76f7e68 Compare November 2, 2026 21:40

github-actions Bot added the type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct label Nov 2, 2026

gdevenyi mentioned this pull request Nov 2, 2026

BUG: Make N4BiasFieldCorrectionImageFilter work with GlobalReleaseDataFlag #7093

Open

hjmjohnson added the status:Confirmed Confirmed/reproduced issue on a different machine with same or similar settings to those reported label Nov 3, 2026

hjmjohnson marked this pull request as ready for review November 3, 2026 01:28

greptile-apps Bot reviewed Nov 3, 2026

View reviewed changes

gdevenyi force-pushed the perf-n4-persistent-point-set branch from 76f7e68 to 4c2f1b4 Compare November 3, 2026 02:22

Copilot AI balanced review requested due to automatic review settings November 3, 2026 02:22

Copilot started reviewing on behalf of gdevenyi November 3, 2026 02:22

View session

Copilot AI reviewed Nov 3, 2026

View reviewed changes

hjmjohnson requested changes Nov 3, 2026

View reviewed changes

hjmjohnson marked this pull request as draft November 3, 2026 03:08

hjmjohnson removed the status:Confirmed Confirmed/reproduced issue on a different machine with same or similar settings to those reported label Nov 3, 2026

gdevenyi force-pushed the perf-n4-persistent-point-set branch from 4c2f1b4 to b8322a4 Compare November 3, 2026 03:27

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

gdevenyi force-pushed the perf-n4-persistent-point-set branch from b8322a4 to 683fed9 Compare November 3, 2026 16:41

gdevenyi force-pushed the perf-n4-persistent-point-set branch from 683fed9 to 3feef1f Compare November 4, 2026 00:39

hjmjohnson merged commit 3feef1f into InsightSoftwareConsortium:main Nov 4, 2026
19 checks passed

	/** The fit points of the included voxels, which depend only on the mask, the confidence image and the geometry. */
	/** Fit points of the included voxels. */

	// Values from ITK main before the fit points were built once per filter update.
	// Regression baseline.


		const typename ImporterType::OutputImageType * parametricFieldEstimate = importer->GetOutput();

		auto & values = fitPoints.values->CastToSTLContainer();

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gdevenyi commented Nov 2, 2026

Uh oh!

gdevenyi commented Nov 2, 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!

greptile-apps Bot 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!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

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!

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!

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!

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