Reach PV360 raw data through Study, and read traj from an archive (A3, R4) - #162
Merged
headmeister merged 7 commits intoJul 26, 2026
Merged
headmeister merged 7 commits into
headmeister merged 7 commits into
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/pv360-study-access-and-archive-traj
branch
from
July 26, 2026 04:18
03bcbe5 to
10d7939
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 A3 and R4: PV360 raw data is unreachable through
Study, and atrajcannot be read from an archive.Stacked on #156–#160, #161.
A3 —
Study.get_dataset()on a PV360 studyfolders.pyreturnedexp["fid"]unconditionally. §13.1: ParaVision 360 writesno file named
fid— raw data israwdata.<title>:It now falls back to the lowest-numbered
rawdata.jobNand re-raises when thereis no raw data at all.
R4 —
trajin a.zip/.PvDatasetsbrukerapi/paths.pyexists so that azipfile.Pathcan stand in for a realpath, and provides
file_size()for exactly this case. The fourtrajrecipescalled
os.stat(self.path).st_size, which needs__fspath__:Worse than the loud failure: a radial or spiral
fidread from an archive loadsfine but silently loses its trajectory, because the failure is downgraded to
a
RuntimeWarninganddataset.trajthen raisesTrajNotLoadedas if noneexisted.
The 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.
Verification
Corpus load unchanged (3,197/3,468). New synthetic tests: a PV360 study whose
experiment has only
rawdata.job0, and a radial experiment zipped into anarchive; both fail without the change.