Label the acquisition-object axis for what it is, and keep slice distance a double (R2, R5, R6, D4) - #163
Merged
headmeister merged 8 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
`Study.get_dataset(exp_id)` returned `exp["fid"]` unconditionally. Spec 13.1: ParaVision 360 writes no file named `fid` -- raw data is `rawdata.<title>` -- so the call raises KeyError on every PV360 study; 85 corpus experiments have no fid, 65 of them have rawdata.jobN, and 16 studies are unusable through this API. It now falls back to the lowest-numbered rawdata job and re-raises when there is no raw data at all. The traj recipes sized their file with `os.stat(self.path).st_size`. brukerapi.paths exists precisely so a `zipfile.Path` can stand in for a real path, and it provides `file_size()` for this case; `os.stat` needs `__fspath__`, which an archive member does not have. Reading a traj out of a `.zip`/`.PvDatasets` therefore failed with a bare TypeError, and -- worse -- a radial or spiral fid read from an archive loaded fine but silently lost its trajectory, because the failure is downgraded to a warning and `dataset.traj` then raises TrajNotLoaded as if none existed. The four recipes now call `file_size`, which is exposed to the recipe namespace. Verified on a zipped experiment: the traj reads (2, 978, 16) float64, identical to the filesystem read, and the fid keeps it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
gdevenyi
force-pushed
the
fix/axis-labels-and-slice-distance
branch
from
July 26, 2026 04:18
10112e6 to
057c05c
Compare
…ance a double Four metadata defects, none of which changes an array value. * The 3-D radial layout declares a six-axis k-space (read, projection, partition, NI, NR, receivers) but carried only five dim_type labels, so NI was labelled `repetition`, NR `channel`, and the receiver axis was unlabelled. Schema construction only warns, and `to_kspace(bart=True)` refuses the array. 18 of 1,463 corpus fids are affected, and this was the only remaining length mismatch in the corpus. * The `#NI` axis was labelled `slice` everywhere. Spec 5.2 makes NI the count of acquisition *objects*: NI x NR = NSLICES x ACQ_n_echo_images x ACQ_n_movie_frames x cycles. 60 corpus fids have NI > NSLICES -- an MSME with 15 slices x 24 echoes reports 360 "slices" -- so a caller that slices on the labelled axis gets object k = (slice k//24, echo k%24). The label is now `object`; it maps to the same BART dimension, so array layouts do not move. * Spectroscopy labelled its NI axis `repetition` and its NR axis `average`. NA is co-added during acquisition and is not a stored axis at all (spec 5.2). On a 25-repetition dynamic PRESS series BART put the 25 points on AVG_DIM, where averaging over them destroys the series; they now land on TIME_DIM. * SlicePackageSplitter truncated VisuCoreSlicePacksSliceDist to int, though spec 7.10 declares it double[]: 1.5 mm became 1 and 0.7 mm became 0, unconditionally, for all 90 multi-package corpus datasets. The committed property references pick up the label rename and nothing else; the corpus loads 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
gdevenyi
force-pushed
the
fix/axis-labels-and-slice-distance
branch
from
July 26, 2026 04:41
057c05c to
675cfdc
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 R2, R5, R6 and D4 — four metadata defects, none of which
changes an array value.
Stacked on #156–#162.
R2 — the 3-D radial layout is one label short
It declares a six-axis k-space (read, projection, partition,
NI,NR,receivers) but carried five
dim_typelabels, soNIwas labelledrepetition,NRchannel, and the receiver axis was unlabelled. Schemaconstruction only warns:
18 of 1,463 corpus fids; a corpus sweep confirms this was the only remaining
length mismatch.
R5 — the
NIaxis is not a slice axis§5.2 makes
NIthe count of acquisition objects:NI × NR = NSLICES × ACQ_n_echo_images × ACQ_n_movie_frames × cycles. 60corpus fids have
NI > NSLICES— an MSME with 15 slices × 24 echoes reports 360"slices" — so a caller that slices on the labelled axis gets object
k = (slice k//24, echo k%24). The label becomesobject; it maps to the sameBART dimension, so array layouts do not move.
R6 — spectroscopy labels
NIrepetitionandNRaverageNAis co-added during acquisition and is not a stored axis at all (§5.2). On a25-repetition dynamic PRESS series:
A caller averaging over the
averageaxis destroyed the series it should havekept.
D4 — slice distance truncated to
int§7.10 declares
VisuCoreSlicePacksSliceDistasdouble[].SlicePackageSplittercast it toint, unconditionally, for all 90multi-package corpus datasets: 1.5 mm became 1, and 0.7 mm became 0.
Verification
Corpus load unchanged (3,197/3,468); no shapes change. The committed property
references pick up the label rename (84
slice→object, 16 spectroscopyrepetition/average→object/repetition) and nothing else. New synthetictests cover the 3-D radial label set, the spectroscopy BART placement and the
fractional slice distance.