Fix the API surface: metadata groups, reports, CLI options, version gates (A5-A9, D9, D10, R7, R9, J6, J7) by gdevenyi · Pull Request #164 · isi-nmr/brukerapi-python

gdevenyi · 2026-07-26T04:11:32Z

Fixes A5–A9, D9, D10, R7, R9, J6 and J7 — the
contained API and robustness findings. No pixel value changes.

Stacked on #156–#163.

A5 — Dataset.metadata. Grouping purely by name prefix left the §7.8
equipment bucket structurally empty (no parameter is called VisuEquipment*),
matched VisuManufacturer/VisuInstitution/VisuStation and the §7.7
VisuExperimentNumber/VisuProcessingNumber to no group at all, and omitted
the §7.1 administration group. Groups a prefix cannot identify now list their
members by name, and a prefix must end on a word boundary —
VisuAcquisitionProtocol was reported as visu_acq.uisition_protocol.
Grouped metadata also reaches a report now: _encode_property recurses into
dicts, which used to fail with Object of type ndarray is not JSON
serializable.
A6 — CLI. -i <dir> -o <path that is not an existing dir> matched no
branch and exited 0 having done nothing. -f yml was ignored for a single
dataset, because Dataset.report had no format parameter and always appended
.json. bruker split -o out/ parsed path_out and never passed it on, so
it wrote into the input tree and left out/ empty.
A7 — version gating. Exact VisuCreatorVersion/ACQ_sw_version string
matches left 405 of 2,988 corpus visu_pars falling through every branch —
all PV7, all PV360, and <5.1;5.1>/<6.0.1;6.0.1>, which are the same
scanner writing two creator entries (§7.1) — and PV-360.2.x/4.x falling
through the fid and rawdata dtype branches. Both sides normalise once into
pv_version and compare on the parsed major version.
A8 — Folder.to_json() called itself forever; it now serialises a new
to_dict(), which report(write=False) also uses so it yields dicts rather
than JSON strings.
A9 — a split slice package was constructed from DEFAULT_STATES only,
dropping the parent's scale/combine_complex/property_files; and
methreco/pvmeta — PROCNO files documented in §13/§13.2 and present in the
corpus — had no RELATIVE_PATHS entry, so add_parameter_file raised
KeyError.
D9 — a random-access selection of only the real or only the imaginary
component of a complex 2dseq raised InvalidDataset.
D10 — the synthetic axis a single-slice 2dseq gets was labelled
spatial, so dim_type[encoded_dim:] — what frame-group lookups use —
started with a bogus encoding axis (946 corpus datasets). It is labelled
frame.
R7 — a field map's fourth axis counts echo images, not repetitions, and
NR is a stored axis that block_count did not count at all — masked by
every corpus field map having NR = 1. The layout gains its repetition axis
and the echo axis is labelled echo, which BART maps to TIME2.
R9 — a fid.spiral/fid.navFid companion is a stream of PVM_DigNp-point
acquisitions (§3.5); where that divides the file exactly it is handed over as
(sample, acquisition) rather than as a flat vector.
J6/J7 — the record stream dropped its last chunk unconditionally, so a
file that does not end at ##END= lost its final parameter silently (§2.1
warns about exactly that); and a value that does not fill its declared size,
or a size bracket that is not an integer, raised a raw numpy/int ValueError
instead of InvalidJcampdxFile.

Verification

Corpus-wide the only change is the 20 field maps gaining their singleton
repetition axis; load outcomes are unchanged (3,197/3,468). 10 new synthetic
tests in test/test_api.py, plus tests for R7, R9, J6 and J7; all fail without
the change. Documentation updated for the renamed axis, the metadata groups and
the version gating.

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

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

gdevenyi · 2026-07-26T04:14:52Z

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

…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

…ates A batch of contained API defects, none of which changes a pixel value. * `Dataset.metadata` grouped purely by name prefix, so the 7.8 equipment bucket was structurally always empty (no parameter is called VisuEquipment*), VisuManufacturer/VisuInstitution/VisuStation and the 7.7 VisuExperimentNumber/VisuProcessingNumber matched no group at all, and the 7.1 administration group was missing. Groups that a prefix cannot identify now list their members by name, and a prefix has to end on a word boundary -- `VisuAcquisitionProtocol` was being reported as `visu_acq.uisition_protocol`. Grouped metadata also reaches a report now: `_encode_property` recurses into dicts, which used to fail with "Object of type ndarray is not JSON serializable". * CLI: `-i <dir> -o <path that is not an existing dir>` matched no branch and exited 0 having done nothing; the output directory is created. `-f yml` was ignored for a single dataset because `Dataset.report` had no format parameter and always appended `.json`; it takes `format_` and rejects anything but json/yml instead of silently writing nothing. `bruker split -o out/` parsed `path_out` and never passed it to the splitters, so it wrote into the input tree and left `out/` empty. * Version gating compared exact `ACQ_sw_version`/`VisuCreatorVersion` strings, so 405 of 2,988 corpus visu_pars -- every PV7 and PV360 file, and `<5.1;5.1>`/`<6.0.1;6.0.1>`, which are the same scanner writing two creator entries (spec 7.1) -- fell through every branch, and PV-360.2.x/4.x fell through the fid and rawdata dtype branches. Both sides now normalise once into `pv_version` and compare on the parsed major version. * `Folder.to_json()` called itself forever; it now serialises a new `to_dict()`, which `report(write=False)` also uses so it yields dicts rather than JSON strings. * A split slice package was constructed from DEFAULT_STATES only, dropping the parent's scale/combine_complex/property_files; it inherits them. `methreco` and `pvmeta` -- PROCNO files documented in 13/13.2 and present in the corpus -- had no RELATIVE_PATHS entry, so add_parameter_file raised KeyError. * A random-access selection of only the real or only the imaginary component of a complex 2dseq raised InvalidDataset; there is nothing to combine, so the axis is left in place. * The synthetic axis a single-slice 2dseq gets was labelled `spatial`, which made `dim_type[encoded_dim:]` -- what frame-group lookups use -- start with a bogus encoding axis. It is labelled `frame` (946 corpus datasets take this path). * A field map's fourth axis counts echo images, not repetitions, and NR is a stored axis that block_count did not count at all -- masked by every corpus field map having NR = 1. The layout gains its repetition axis and the echo axis is labelled `echo`, which BART maps to TIME2. * A `fid.spiral`/`fid.navFid` companion is a stream of PVM_DigNp-point acquisitions (spec 3.5); where that divides the file exactly it is handed over as (sample, acquisition) rather than as a flat vector. * JCAMP-DX robustness: the record stream dropped its last chunk unconditionally, so a file that does not end at `##END=` lost its final parameter silently (spec 2.1 warns about exactly that); and a value that does not fill its declared size, or a size bracket that is not an integer, raised a raw numpy/int ValueError instead of InvalidJcampdxFile. Corpus-wide the only change is the 20 field maps gaining their singleton repetition axis; load outcomes are 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 and others added 4 commits July 25, 2026 22:58

Copilot AI review requested due to automatic review settings July 26, 2026 04:11

Copilot AI reviewed Jul 26, 2026

gdevenyi and others added 3 commits July 26, 2026 00:18

gdevenyi force-pushed the fix/api-correctness-and-robustness branch from 5c6222d to f2890c9 Compare July 26, 2026 04:18

gdevenyi and others added 2 commits July 26, 2026 00:26

gdevenyi force-pushed the fix/api-correctness-and-robustness branch from f2890c9 to e2f44d1 Compare July 26, 2026 04:41

headmeister merged commit 3540a5c into isi-nmr:master Jul 26, 2026
8 checks passed

Navigation Menu

Fix the API surface: metadata groups, reports, CLI options, version gates (A5-A9, D9, D10, R7, R9, J6, J7) - #164

Fix the API surface: metadata groups, reports, CLI options, version gates (A5-A9, D9, D10, R7, R9, J6, J7)#164
headmeister merged 9 commits into
isi-nmr:masterfrom
gdevenyi:fix/api-correctness-and-robustness

gdevenyi commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

gdevenyi commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gdevenyi commented Jul 26, 2026

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

gdevenyi commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants