Skip to content

Measure the slice extent along the slice axis, not along patient z - #209

Merged
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/extent-slice-axis
Aug 10, 2026
Merged

Measure the slice extent along the slice axis, not along patient z#209
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/extent-slice-axis

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #194.

"cmd": "np.array([#VisuCoreExtent[0], #VisuCoreExtent[1], (np.shape(#VisuCorePosition)[0] + 1 ) * abs(#VisuCorePosition[1,2] - #VisuCorePosition[0,2])])"

Two errors in one expression: only the z component of the slice step is used, and the step is counted N + 1 times.

Spec 7.2 makes VisuCorePosition a 3-vector in patient coordinates; spec 5.4/12 let the slice normal point anywhere (ACQ_slice_orient ranges over sagittal, coronal and Arbitrary_Oblique, and the normal is the third row of VisuCoreOrientation). Projecting onto z alone is zero whenever the stack is not axial.

Before / after

.../11/pdata/1  step [0,-1,0]  10 slices x 1mm   extent [20, 20,  0.0] -> [20, 20, 10.0]
.../6/pdata/1   step [0, 0,1]  10 slices x 1mm   extent [20, 20, 11.0] -> [20, 20, 10.0]

Corpus sweep over pv5, pv6, pv7, new-orientation:

before: 77 multi-slice 2-D reconstructions, 14 with extent[2] == 0, 63 off by one slice
after : 77 correct, 0 wrong

Change

np.shape(#VisuCorePosition)[0] * np.linalg.norm(#VisuCorePosition[1,:] - #VisuCorePosition[0,:]) — the length of the whole difference, which is what the sibling resolution recipe and Dataset.slice_distance already use, times the number of slices, which keeps extent = size × resolution as it is in plane.

The corpus property references move with it: 54 extent values across the four test/config/properties_*.json, regenerated in place so key order and layout are unchanged.

Test

test_extent_spans_the_slice_axis_whatever_direction_it_points builds a synthetic 5-slice stack for an axial, coronal, sagittal and oblique normal and asserts extent[2] == n × step, agreeing with resolution[2] and slice_distance. Three of the four parameters fail on master.

Suite: 2140 passed, 12 skipped.

gdevenyi and others added 2 commits August 8, 2026 16:33
`ruff format --check` reported twelve files as unformatted, so any change
touching one of them dragged unrelated reflow into its diff. Run the
formatter once, on its own, so subsequent changes show only what they
actually change.

Formatting only: no behaviour changes, and `ruff check` is clean before and
after. The suite is unchanged at 2140 passed, 12 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
The extent recipe built its third component from
abs(VisuCorePosition[1,2] - VisuCorePosition[0,2]) -- the patient-z component
of the step between two slice centres -- and multiplied it by the number of
positions plus one.

Spec 7.2 makes VisuCorePosition a 3-vector in patient coordinates, and spec
5.4/12 let the slice normal point anywhere: ACQ_slice_orient ranges over
sagittal, coronal and Arbitrary_Oblique, and the normal is the third row of
VisuCoreOrientation. Projecting onto z alone therefore reports exactly zero
for every coronal, sagittal or oblique stack, and the +1 overstates the
axial ones by one slice.

Across the pv5, pv6, pv7 and new-orientation corpora all 77 multi-slice 2-D
reconstructions were wrong: 14 reported 0.0, the other 63 were long by one
slice thickness. Both now report n * step, the length of the whole
difference -- which is what the sibling resolution recipe and
Dataset.slice_distance already use, and which keeps extent = size *
resolution as it is in plane.

The corpus property references move with it, 54 extent values in four files.

Closes isi-nmr#194

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
@gdevenyi
gdevenyi force-pushed the fix/extent-slice-axis branch from 6da2e4c to dbec286 Compare August 8, 2026 20:51
@gdevenyi

gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto #213 (ruff format), so this branch carries that commit until #213 merges — merge #213 first and this diff reduces to its own change. Verified after the rebase: ruff check and ruff format --check clean, full suite green.

Integration check across all eleven conformance PRs merged together: 2164 passed, 12 skipped; corpus load test 3202 → 3207 of 3478 with zero newly broken datasets.

@headmeister
headmeister merged commit 77f6954 into isi-nmr:master Aug 10, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

extent[2] is derived from the z-component of the slice step and counts N+1 slices, so every 2-D multi-slice stack is wrong (spec 7.2, 12)

2 participants