Skip to content

STYLE: Share the MINC apparent-dimension read between time and space - #6875

Open
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-minc-extract-apparent-dim-helper
Open

hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-minc-extract-apparent-dim-helper

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Sep 18, 2026

Copy link
Copy Markdown
Member

Extracts the duplicated MINC apparent-dimension read in ReadImageInformation into one helper. No functional change; addresses item 4 of #6874.

What was duplicated

The time-dimension block added by #6586 repeats the spatial loop's body nearly verbatim: attach the file dimension to the next apparent-order slot, request MI_POSITIVE voxel order, then read size, separation, and start in apparent order.

Both call sites now use:

const MINCApparentDimensionGeometry geometry =
  MINCAttachApparentDimension(m_MINCPImpl->m_MincApparentDims, usableDimensions, fileDim);

The spatial site keeps its own miget_dimension_cosines call, since the time axis derives its direction from a unit vector rather than from stored cosines.

The vector_dimension block is deliberately left alone: it does not request positive voxel order (a vector dimension has no meaningful step direction), so folding it into the helper would change behavior.

Verification

Built on main after #6586 merged, Release, clang, Module_ITKIOMINC=ON and Module_ITKIOTransformMINC=ON.

  • ITKIOMINC forced full recompile: 0 warnings, 0 errors.
  • ctest -R MINC — 125/125 passed (110 ITKIOMINC, 4 ITKIOTransformMINC, plus style/Doxygen checks).
  • pre-commit run --all-files — exits 0, all hooks including KWStyle.

@github-actions github-actions Bot added 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:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming) labels Sep 18, 2026
@hjmjohnson
hjmjohnson force-pushed the enh-minc-extract-apparent-dim-helper branch from 0476aba to 275d038 Compare September 18, 2026 14:54
@github-actions github-actions Bot removed 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 Sep 18, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review September 18, 2026 14:58
@hjmjohnson
hjmjohnson requested a review from gdevenyi September 18, 2026 14:58
@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

Safe to merge.

Summary

This update removes the redundant helper comment while retaining the shared MINC apparent-dimension setup and geometry handling.

Reviews (2) · Last reviewed commit: "STYLE: Share the MINC apparent-dimension..."

Comment thread Modules/IO/MINC/src/itkMINCImageIO.cxx Outdated
The time-dimension block repeated the spatial loop's handle attach,
positive-order request, and size/separation/start reads verbatim. A
single helper keeps the two call sites from drifting apart.

The vector_dimension block deliberately omits the positive-order
request, so it is left alone.
@hjmjohnson
hjmjohnson force-pushed the enh-minc-extract-apparent-dim-helper branch from 275d038 to 3d96eb2 Compare September 18, 2026 15:07
@hjmjohnson

Copy link
Copy Markdown
Member Author

@greptileai please re-review — the redundant comment was removed in 3d96eb2; the prior review covered 275d038.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant