Read transposed frames in their stored shape and own frame-group metadata by descriptor (D1, D2) - #161
Merged
headmeister merged 6 commits intoJul 26, 2026
Conversation
FILE_FORMAT.md 7.2 defines VisuCorePosition as the centre of the first pixel/voxel transferred and VisuCoreOrientation as the patient -> image matrix (i = M.p), so a voxel-index -> patient affine must map index (0,0,0) onto VisuCorePosition[0]. Four defects meant it never did: * the `position` recipe added a whole in-plane field of view to the origin, displacing every image dataset (median 35 mm, max 126 mm); * `position_matrix` re-applied VisuSubjectPosition on top of Visu parameters that are already in the DICOM patient frame, mirroring x and y for every Head_Supine dataset -- and only the linear part, so the columns and the translation lived in different frames. Spec 12 puts ACQ_patient_pos on the magnet -> patient leg, which Visu has already traversed, and allows only the fixed diag(-1,-1,1) pair applied to both ends; * slice spacing added VisuCoreFrameThickness to the already centre-to-centre VisuCoreSlicePacksSliceDist (doubling it), used the z component alone on PV5.1 (zero, hence a singular affine, for any sagittal or coronal stack), and was never signed, so stacks that advance against the third row of the orientation matrix came out reversed; * spectroscopic and CSI datasets fell through every branch to an unconditional np.identity(4), which is indistinguishable from a real affine. Spec 7.2 says such frames must be detected and skipped. The branching this needs is beyond what the recipe language expresses cleanly, so the derivation moves into Python: Dataset.affine_of_package() builds the transform for one slice package, Dataset.slice_packages_index() resolves package boundaries (including the PV5.1 case, which defines none of the 7.10 parameters, by grouping frames that share an orientation), and Dataset.affine returns the first package's transform, warns when a single affine cannot describe the dataset, and raises UnsupportedDatasetType for frames that are not purely spatial. The slice column is the measured step between slice centres, which carries both direction and spacing; the vendor slice distance and frame thickness are fallbacks for a single-slice package. Geometry follows the data when VisuCoreDiskSliceOrder reverses the stored frame order. Verified over the review corpus: 1591/1591 image 2dseq now satisfy affine @ (0,0,0,1) == VisuCorePosition[0] (previously 0), no affine is singular (previously 23), every slice index maps onto its own position wherever the slices are collinear, and the 35 spectroscopy datasets that used to receive an identity matrix now refuse. Reports keep carrying the affine; the position, position_matrix and rotation intermediates are gone, so the committed property references are regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
…orrupting records ParaVision hard-wraps a value block near column 80 by INSERTING a newline (FILE_FORMAT.md 2.2); it deletes nothing. Two places got that backwards. Reading: `_normalize_line_breaks` replaced the newline and the blanks around it with a single space. Where the writer broke after a space that is right by accident, but where the break fell mid-token a space is manufactured out of nothing -- and leading blanks that are part of the value are eaten. Corpus evidence for the direction of the fix: across a 1,500-file sample, 15,420 records are wrapped at a non-space character and *zero* wrap a struct-tuple boundary without keeping the space. So RF pulse shapes came back as `'< gauss.exc>'`, coil elements as `'<1H >'`, coil serials as `'<T11204V3 >'`, and the CONFIG_SCAN_* blob gained a space after every wrap. This reverses the recommendation in issue isi-nmr#102, which proposed normalizing to a space; the on-disk evidence above says the newline must simply be deleted. Writing: `wrap_lines` split each over-long line on whitespace and rebuilt it with single spaces, so it deleted the break character and collapsed blank runs -- and it wrapped `$$` comment lines too, whose tail then no longer starts with `$$` and is read back as value data of the preceding parameter (spec 2.1). With 7,385 of 10,720 corpus files carrying a `$$` line longer than 78 characters, `Dataset.write()` was routinely emitting a file that changed a parameter on re-read: over a 400-file sample, 289 files came back with a corrupted record (`OWNER` picking up the path from the comment line below it) and only 108 files were even a fixed point. Wrapping now inserts newlines, never touches a comment, and hard-breaks only a token longer than the line limit -- which the corrected read path rejoins exactly. The comment records that belong to no parameter -- the last `$$ @vis=` block and the `$$ File finished by PARX` trailer -- were dropped on read and so lost on write; they are now kept and re-emitted around `##END=`, and the file ends with a newline as ParaVision writes it. Over the same 400-file sample all 400 files now round-trip with every record identical, all 400 are fixed points, and 269 are byte-identical to the vendor file. Over 1,500 files: 1,497/1,497 records identical. Two existing unit tests encoded the space-substitution assumption with inputs that do not occur on disk (a value block wrapped with no trailing space); they now use the wrapped-after-a-space form ParaVision writes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
Three parsing defects, all silent, all in the same tokenizer:
* `<...>` matching used `<[^<>]*>` and kept only what matched, throwing the
rest of the record away. FILE_FORMAT.md 2.2/10.1 document `\<`/`\>` as
escaped characters -- ParaVision writes them in the reco filter graph --
so `(<input>, 0, <Q-\>S>)` came back as `['<input>', 0, '<Q-\>']` with
the destination silently dropped, and a `RecoStageNodes` descriptor was
replaced by the fragment `'<BYTORDA\>'`. 1,542 corpus files carry such an
escape; the recorded reconstruction pipeline was unrecoverable from the
parsed value for every PV6/PV7 `reco`.
A backslash is not always an escape: an empty study description is
written `<\>`, where reading `\>` as escaped leaves the string open. The
escaped reading is therefore tried first and a string that never
terminates under it is re-read with the backslash as content.
* The struct splitter tracked `<>` depth but not `()` depth, so a nested
tuple (spec 2.3) was cut in half and its parentheses glued onto the
neighbouring tokens: `AdjKnownList[0]` -- in 1,448 corpus files -- read
`['(EMPTY', ..., 'HANDLE_ACQUISITION)', 'No', 'No']` instead of
`[['EMPTY', ..., 'HANDLE_ACQUISITION'], 'No', 'No']`. The element count
stayed right, so nothing raised.
* Any record shaped `(((...)...)...)` was routed to a GeometryParameter
whose `value` is `None`, whose `to_dict` is `{}` and which never defines
`size`, so `get_array` raised an untyped AttributeError. Spec 2.2/2.3
give those records no special status, and 5.4/12 make their content
load-bearing: the leading `((R9, T3), extent, axis-labels, id)` of
`PVM_SliceGeo` is the rotation matrix and offset of the slice geometry.
1,505 corpus files carry one. With the splitter fixed they are ordinary
nested structs, so the special case is deleted.
Swept over a 1,200-file sample: 174,305 parameter values parse, none
raises, none is lost, and exactly 1,060 values change -- all of them
geometry objects that used to be None (PVM_SliceGeo, PVM_FovSatGeoCub,
PVM_MapShimVolumes, ...) or the escape/nesting cases above. The corpus
load result is unchanged at 3,197/3,468.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
Two documented features that could not work at all. `FrameGroupSplitter.split()` built a path whose last segment is the *file* name -- `<procno>_FG_ECHO_<n>/2dseq` -- and then created it with os.makedirs, so a directory sat exactly where the 2dseq file belongs. `split(write=True)` therefore always failed with IsADirectoryError, which makes `bruker split --frame_group` dead on arrival, and because the makedirs ran unconditionally the pure in-memory path (`write=False`) had a filesystem side effect that littered the dataset tree with stray `2dseq` directories -- Dataset() then rejects each one with NotADatasetDir. A load=0 Dataset does not need its path to exist, so the directory creation is simply gone, and Splitter.write() now creates the parent directory of its target instead. `add_parameters=` was accepted by Dataset() and stored as an inert state key: `_read_parameters` merged only `parameter_files` and `optional_parameter_files`. Every caller that asked for the study `subject` file (spec 9/7.5) -- `bruker report`, Folder.report(), Filter -- silently got a dataset without it, so no report carried subject identity and a fid `id` degenerated to `FID_<expno>__`, which made `Folder.report(path_out=...)` write the same filename for every study and overwrite. The keyword is now honoured. Folder's filter used a second misspelling, `add_properties`, and is corrected to the keyword that exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
Contributor
Author
|
Part of the review series tracked in #171. |
FILE_FORMAT.md 3.1: with `GO_block_size = continuous` the block holds `ACQ_size[0] x Nchan` words and all of them are digitized data. EPSI had its own `acq_length` of `2 * PVM_DigNp * channels // NSegments`, one NSegments-th of the block, while `block_count` already multiplied by NSegments -- so the reader walked past all but the last segment of every block and nothing checked that the layout accounted for the file. On pv6 lego_phantom/34 (ACQ_size=[12288 4 64], PVM_DigNp=6144, NSegments=4) that used 786,432 of 3,145,728 words: 25 % of the acquisition, with the discarded head carrying three times the energy of the kept tail. The k-space came out with a spectral axis of 64 instead of 256. It loads without error, so it is silent. EPSI now gets its own branch, ahead of the dEPI ones, for block_count, encoding_space, permute, k_space and dim_type, and the EPSI-specific acq_length is gone so the generic continuous branch applies. The result is k_space = (PVM_EncMatrix[0], ACQ_size[2], NSegments * PVM_DigNp / PVM_EncMatrix[0], receivers) -- (96, 64, 256, 1) on that dataset, which matches both `RECO_inp_size = (0, 256, 64)` and the vendor 2dseq element count 96*256*64, with 3,145,728 of 3,145,728 words used. Across the corpus exactly the two EPSI fids change shape and no dataset changes load outcome (3,197/3,468 as before). The spectral interleave is taken to run `spectral * NSegments + segment`; the total sample count is fixed either way, but if the vendor order is the reverse the spectral axis is permuted, which wants a dataset with a known spectrum to settle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
…data by descriptor Two independent silent-wrong-data defects in the 2dseq path. VisuCoreTransposition was never read. Spec 7.2 makes it per frame: a nonzero value means that frame is stored with two of its dimensions exchanged relative to VisuCoreSize, so reshaping it with VisuCoreSize interleaves its rows. On a 110x120 mixed-transposition localizer the affected frames come out as diagonal-stripe noise; sampling the intersection line of an untransposed and a transposed frame correlates -0.27 as delivered and +0.64 once the exchange is undone (and +0.13 -> +0.86 on another pair). Schema2dseq now reads each such frame in its real on-disk shape and swaps it back, and inverts that on write so the binary round-trip stays bit-exact. The exchange is skipped when the two dimensions have equal length. That is where this departs from the literal spec text, and it is measured: on a 256x256 three-package localizer whose middle package carries transposition=1, the delivered frames already agree with VisuCoreOrientation (cross-plane correlation 0.99, 0.94) and applying the swap destroys that agreement (-0.05, -0.28). 89 of the 90 corpus datasets with a nonzero transposition are square, so this keeps them untouched while fixing the one that is genuinely scrambled. Whether Bruker's own export presents square transposed frames with row-swapped orientation matrices instead is unresolved; the measurement above is what this follows. frame_group_values misread VisuGroupDepVals. Spec 7.4 says ownership runs from the frame-group descriptor, whose (valsStart, valsCnt) is a window into VisuGroupDepVals; VisuGroupDepVals[k][1] is a start index into the dependent *parameter* array, and it is almost always 0. Reading it as an index into VisuFGOrderDesc therefore assigned every dependent parameter to frame group 0, and a size-matching rescue hid that whenever exactly one axis had the right length. 139 of 583 corpus datasets were assigned differently from the spec; 4 of those cannot be rescued by size at all. On a PV7 3-echo x 3-slice scan the three slice positions were broadcast along the echo axis; they now land on the slice axis and the echo times on the echo axis. Random access records which absolute frames a selection covers, so a per-frame parameter is indexed by frame number rather than by position within the selection. Corpus-wide: no dataset changes load outcome or shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
gdevenyi
force-pushed
the
fix/frame-transposition-and-group-ownership
branch
from
July 26, 2026 04:18
7dff4a8 to
d7a90d3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes D1 and D2: two independent silent-wrong-data defects in the 2dseq
path.
Stacked on #156–#160.
D1 —
VisuCoreTranspositionis never read§7.2 makes it per frame: a nonzero value means that frame is stored with two of
its dimensions exchanged relative to
VisuCoreSize, so reshaping it withVisuCoreSizeinterleaves its rows. On a 110×120 mixed-transposition localizer(
zenodo/samri_paravision/…/5/pdata/1) the affected frames are delivered asdiagonal-stripe noise. Sampling the intersection line of an untransposed and a
transposed frame and correlating the two profiles:
Schema2dseqnow reads such a frame in its real on-disk shape and swaps itback, and inverts that on write so the binary round-trip stays bit-exact.
The exchange is skipped when the two dimensions have equal length, which is
where this departs from the literal spec text — and it is measured. On the
256×256 three-package localizer
pv6/…/lego_phantom/8, whose middle packagecarries
transposition = 1, the delivered frames already agree withVisuCoreOrientationand applying the swap destroys that agreement:89 of the 90 corpus datasets with a nonzero transposition are square, so this
keeps them untouched while fixing the one that is genuinely scrambled. Whether
Bruker's own export presents square transposed frames with row-swapped
orientation matrices instead is unresolved; the measurement above is what this
follows, and the reasoning is in the code comment.
D2 —
frame_group_valuesmisreadsVisuGroupDepVals§7.4: ownership runs from the frame-group descriptor, whose
(valsStart, valsCnt)is a window intoVisuGroupDepVals.VisuGroupDepVals[k][1]is a start index into the dependent parameter array,and it is almost always
0. Reading it as an index intoVisuFGOrderDescassigned every dependent parameter to frame group 0; a size-matching rescue hid
that whenever exactly one axis had the right length.
139 of 583 corpus datasets were assigned differently from the spec; 4 of those
cannot be rescued by size at all.
Random access also records which absolute frames a selection covers, so a
per-frame parameter is indexed by frame number rather than by position within
the selection.
Verification
Corpus-wide no dataset changes load outcome or shape (3,197/3,468). New
synthetic tests in
test/test_frames.pycover the non-square exchange, thesquare no-op, the write round-trip, random access, and descriptor-window
ownership. One existing unit test parametrized
RECO_transpositionandVisuCoreTranspositiontogether, asserting neither changes the data; §6.9 and§7.2 make them different things, so it now covers
RECO_transpositionalone.