Skip to content

BUG: Propagate rank to the last axis in FastApproximateRank#6580

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
physwkim:bug-fast-approximate-rank-last-axis
Jul 10, 2026
Merged

BUG: Propagate rank to the last axis in FastApproximateRank#6580
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
physwkim:bug-fast-approximate-rank-last-axis

Conversation

@physwkim

Copy link
Copy Markdown
Contributor

Addresses item B18 of #6575.

FastApproximateRankImageFilter is a separable mini-pipeline of one per-axis RankImageFilter per image dimension. Its SetRank() forwards the caller's rank to only the first ImageDimension - 1 sub-filters:

for (unsigned int i = 0; i < TInputImage::ImageDimension - 1; ++i)
{
  this->m_Filters[i]->SetRank(m_Rank);
}

so the last axis always runs at the sub-filter default of 0.5 — a median — regardless of the requested rank, and for 1-D images the loop body never runs at all. GetRank() still reports the requested value, so the mismatch is silent. The fix is the loop bound: ImageDimension - 1ImageDimension (MiniPipelineSeparableImageFilter holds exactly ImageDimension sub-filters, one per axis).

The new regression test isolates the last axis: a 7×7 image constant along axis 0 and strictly increasing along axis 1, radius (1, 1), rank 0.02. The axis-0 pass is an identity on constant data, so the axis-1 (last-axis) pass is the discriminator: per RankHistogram's selection rule (target = rank * (entries - 1) + 1), rank 0.02 over a 3-sample window selects the minimum while the default 0.5 selects the median. Interior pixels must equal the window minimum 100·y; unfixed, they come out as the median 100·(y+1).

Verified locally (Linux, GCC 13, Release): the new test fails on unmodified main (interior pixels 200, expected 100 — the last axis still ran at rank 0.5) and passes with the fix; the full ITKReview suite plus the ITKImageStatistics, ITKLabelVoting, ITKDisplacementField, and ITKRegionGrowing suites pass with the fix applied.

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)
AI assistance
  • Tool: Claude Code (claude-fable-5, with claude-opus-4-8 subagents)
  • Role: implemented the fix and regression test from the analysis previously filed as item B18 of BUG: Findings collected while porting ~60 filters — reported together in one issue #6575; derived the expected test values from RankHistogram's selection rule before implementation
  • Testing: built locally and verified the regression test fails before / passes after the fix; five module test suites pass with the fix
  • Note: clang-format 19.1.7 was not available locally; formatting was hand-matched to ITK style and may need the CI formatter's touch-up

@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 labels Jul 10, 2026
@greptile-apps

This comment was marked as resolved.

@hjmjohnson hjmjohnson marked this pull request as draft July 10, 2026 11:49
@hjmjohnson hjmjohnson force-pushed the bug-fast-approximate-rank-last-axis branch from bf7a532 to e9125ca Compare July 10, 2026 12:16
FastApproximateRankImageFilter is a separable mini-pipeline of one
per-axis RankImageFilter per image dimension (m_Filters has
ImageDimension entries; each sub-filter defaults to rank 0.5, the
median). SetRank() looped over ImageDimension - 1 filters, so the
sub-filter for the last axis never received the caller's rank and
always ran at its default median. For 1-D images the loop body never
executed, so the rank never propagated at all.

Change the loop bound to ImageDimension so every per-axis sub-filter
receives the requested rank.

Add a regression test that drives a 2-D image constant along axis 0
and increasing along axis 1, so only the last-axis pass changes the
result: with rank 0.02 the interior pixels must equal the window
minimum, which fails before the fix (the last axis ran at the default
median) and passes after it.

Addresses item B18 of InsightSoftwareConsortium#6575.
@hjmjohnson hjmjohnson force-pushed the bug-fast-approximate-rank-last-axis branch from e9125ca to 841f4b6 Compare July 10, 2026 21:25
@hjmjohnson hjmjohnson merged commit 5692478 into InsightSoftwareConsortium:main Jul 10, 2026
15 of 18 checks passed
physwkim added a commit to physwkim/sitk-rs that referenced this pull request Jul 12, 2026
Upstream's SetRank forwarded rank to only the first ImageDimension-1
of its per-axis filters, leaving the last axis permanently forced to
RankImageFilter's own built-in default of 0.5 (median) regardless of
the caller's requested rank -- and for 1-D images, whose only axis is
axis 0 == ImageDimension-1, the loop body never ran at all. Fixed per
upstream PR InsightSoftwareConsortium/ITK#6580, which widens the loop
bound to ImageDimension: every axis's separable pass now receives the
same rank.

Rewrote the two tests that exploited the old forced-median default as
a backdoor to pin the median tie-break convention (they now pass
rank=0.5 explicitly instead), and renamed/rederived the two tests that
pinned the bug itself (fast_approximate_rank_1d_honors_the_requested_rank,
fast_approximate_rank_applies_rank_uniformly_to_every_axis_2d) to
assert the corrected separable-max result instead of the old
forced-median one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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