Skip to content

BUG: Preserve OutputRegionMode in IterativeDeconvolutionImageFilter#6599

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
physwkim:bug-iterative-deconvolution-output-region-6575
Jul 14, 2026
Merged

BUG: Preserve OutputRegionMode in IterativeDeconvolutionImageFilter#6599
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
physwkim:bug-iterative-deconvolution-output-region-6575

Conversation

@physwkim

Copy link
Copy Markdown
Contributor

Stop IterativeDeconvolutionImageFilter::GenerateData() from overwriting the output region metadata that GenerateOutputInformation() had already computed, which made OutputRegionMode a no-op. Addresses B17 of #6575.

Root cause

itkIterativeDeconvolutionImageFilter.hxx:113-116:

outputPtr->SetRequestedRegion(inputPtr->GetRequestedRegion());
outputPtr->SetBufferedRegion(inputPtr->GetBufferedRegion());
outputPtr->SetLargestPossibleRegion(inputPtr->GetLargestPossibleRegion());

ConvolutionImageFilterBase::GenerateOutputInformation() (itkConvolutionImageFilterBase.hxx:39-45) has already shrunk the output's largest-possible region for OutputRegionModeEnum::VALID by the time GenerateData() runs — and these three lines overwrite it with the input's (mode-blind) regions. PadInput and CropOutput (itkFFTConvolutionImageFilter.hxx:157, :468) read the output's requested region after the overwrite, so SetOutputRegionModeToValid() was accepted and silently ignored by Landweber, ProjectedLandweber, and RichardsonLucy alike.

The requested region is now derived from the output's own largest-possible region, which already reflects the selected mode. The manual buffered-region assignment and Allocate() were redundant: CropOutput() calls AllocateOutputs(), which sets the buffered region and allocates.

Local validation

New GoogleTest itkIterativeDeconvolutionImageFilterGTest.cxx.

Fail-before (OutputRegionModeValidShrinksOutputRegion): the filter returned the full 10x10 region instead of the expected 8x8 VALID-mode region.

Pass-after:

1/3 IterativeDeconvolutionImageFilterTest.OutputRegionModeSameMatchesInput ......... Passed
2/3 IterativeDeconvolutionImageFilterTest.OutputRegionModeValidShrinksOutputRegion .. Passed
3/3 ProjectedIterativeDeconvolutionImageFilterTest.BasicObjectMethods ............... Passed
100% tests passed

ctest -R Deconvolution (33 tests): identical results with the fix applied and reverted — the 14 legacy baseline tests fail identically in both, on ExternalData images this offline build tree never fetched (ExternalData_URL_TEMPLATES empty in the cache), i.e. before GenerateData() is reached. No baseline result changed because of this fix; CI will exercise them for real.

AI assistance

@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module labels Jul 13, 2026
@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR preserves the deconvolution output region chosen by the convolution base class. The main changes are:

  • Uses the output largest possible region when setting the requested region in GenerateData().
  • Removes manual input-region copying and early output allocation from iterative deconvolution.
  • Adds GTest coverage for SAME and VALID output-region modes.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.

No files need attention.

T-Rex T-Rex Logs

What T-Rex did

  • Saved the earlier raw tooling probe artifact for iterative_deconvolution to establish baseline context.
  • Captured the main tooling blocker check results showing cmake_exit:127 and ninja_exit:127, and that the intended configure/build/ctest commands were not run.
  • Verified that the C++ compiler version check completed successfully.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/Filtering/Deconvolution/include/itkIterativeDeconvolutionImageFilter.hxx Updates GenerateData() to preserve output-region metadata computed before execution.
Modules/Filtering/Deconvolution/test/CMakeLists.txt Adds the new iterative deconvolution GTest source to the module GTest driver.
Modules/Filtering/Deconvolution/test/itkIterativeDeconvolutionImageFilterGTest.cxx Adds tests for SAME and VALID output-region behavior on Landweber deconvolution.

Reviews (1): Last reviewed commit: "BUG: Preserve OutputRegionMode in Iterat..." | Re-trigger Greptile

@hjmjohnson hjmjohnson force-pushed the bug-iterative-deconvolution-output-region-6575 branch from 6b68809 to d20ea33 Compare July 13, 2026 23:57
@hjmjohnson

Copy link
Copy Markdown
Member

A high-effort automated review of this branch raises design questions about the VALID-mode path that deserve author input before merge (force-push so far = plain rebase on main only).

Findings (ranked)
  1. Zero-size VALID region now throws (CONFIRMED): with a kernel larger than the input (e.g. 3×3 image, 7×7 PSF), GetValidRegion() is size-0 and the new SetRequestedRegion(LPR) feeds a zero-size extraction into ExtractImageFilter, which throws — previously such pipelines produced (arguably meaningless) output without error. Worth an explicit itkExceptionMacro with a clear message, or documented rejection.
  2. Pipeline convention: forcing the output requested region inside GenerateData() bypasses region negotiation; the pipeline-sanctioned hook for "this filter always produces its full output" is an EnlargeOutputRequestedRegion() override.
  3. VALID/SAME numerical inconsistency (PLAUSIBLE): the requested-region change makes PadInput crop the kernel-radius padding before the FFT, and since iterative deconvolution is global in the FFT domain, VALID output is not the boundary-independent interior of the SAME output — users comparing the two get different values everywhere. If intended, worth documenting.
  4. Stale estimate cache (PLAUSIBLE): Initialize() keys m_CurrentEstimate only on input MTime; switching OutputRegionMode after an aborted run reuses a padded estimate with the wrong geometry.
  5. Test blindness: both new tests use a constant image + constant kernel and assert region metadata only, so a spatially shifted VALID crop would pass; only odd 3×3 kernels are exercised (the even-kernel and kernel-larger-than-image branches of GetValidRegion() are untested). A ramp input plus one even-kernel case would close this.

GenerateData() overwrote the output region metadata with the input
image's regions, after GenerateOutputInformation had already applied
OutputRegionModeEnum::VALID via GetValidRegion(). PadInput/CropOutput
read the output's requested region after this overwrite, so VALID
mode was silently discarded for every iterative deconvolution filter.

The requested region now derives from the output's own largest
possible region, which already reflects the selected mode. The
manual buffered-region assignment and Allocate() were redundant,
since CropOutput's AllocateOutputs() call performs both.

Addresses item B17 of InsightSoftwareConsortium#6575.
@hjmjohnson hjmjohnson force-pushed the bug-iterative-deconvolution-output-region-6575 branch from d20ea33 to 3343ebb Compare July 14, 2026 01:58
@hjmjohnson

Copy link
Copy Markdown
Member

Force-push = plain rebase onto current main, folding the new GTest source into the GTests list that #6589 introduced for this module (two creategoogletestdriver calls in the CI merge build caused/would cause a duplicate-target configure error). Verified locally: configure clean, module tests pass.

@hjmjohnson hjmjohnson merged commit 763b4b8 into InsightSoftwareConsortium:main Jul 14, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Filtering Issues affecting the Filtering module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants