Read every acquired sample of an EPSI fid (R1) - #160
Merged
Merged
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
This was referenced Jul 26, 2026
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
gdevenyi
force-pushed
the
fix/epsi-uses-every-sample
branch
from
July 26, 2026 04:18
a8cc832 to
7bb20c3
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 R1: EPSI
fiddatasets discard 75 % of the acquired samples, silently.Stacked on #156–#158, #159.
The problem
§3.1: with
GO_block_size = continuousthe block holdsACQ_size[0] × Nchanwords and all of them are digitized data. EPSI had its own
acq_lengthof2 × PVM_DigNp × channels // NSegments— one NSegments-th of the block — whileblock_countalready multiplied byNSegments, and nothing checked that thelayout accounted for the file:
The discarded head carries three times the energy of the kept tail, and the
spectral axis comes out 64 instead of 256. The dataset loads without error: the
size check validates only
block_size × block_count.The fix
EPSI gets its own branch — ahead of the dEPI ones — for
block_count,encoding_space,permute,k_spaceanddim_type, and the EPSI-specificacq_lengthis deleted so the genericcontinuousbranch applies:which matches both
RECO_inp_size = (0, 256, 64)and the vendor2dseqelement count
96·256·64.Verification
Across the corpus exactly the two EPSI fids change shape
(
[96, 64, 1, 1, 1, 64]→[96, 64, 256, 1]) and no dataset changes loadoutcome (3,197/3,468). New synthetic tests in
test/test_raw_layouts.pyassertthat
acq_length × block_countaccounts for the whole file and that everystored complex sample survives.
Open question: the spectral interleave is taken to run
spectral · NSegments + segment. That ¾ of the samples were dropped, and thatthe target is 96×256×64, are both certain; if the vendor order is the reverse
the spectral axis is permuted, which wants a dataset with a known spectrum to
settle.