Skip to content

Derive the fid receiver count from the receiver count, not from ACQ_dim_desc - #214

Merged
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/spectroscopic-channels
Aug 10, 2026
Merged

Derive the fid receiver count from the receiver count, not from ACQ_dim_desc#214
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/spectroscopic-channels

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #202.

{"cmd": "1", "conditions": ["#ACQ_dim_desc.__class__.__name__=='str'", "#ACQ_dim_desc=='Spectroscopic'"]},
{"cmd": "1", "conditions": ["#ACQ_dim_desc[0]=='Spectroscopic'"]},
{"cmd": "#PVM_EncNReceivers", "conditions": ["#ACQ_dim_desc[0]!='Spectroscopic'"]}

ACQ_dim_desc describes a dimension as Spatial or Spectroscopic (spec 5.1) and says nothing about receivers. Spec 3.1 defines Nchan as the number of active receiver channels, with no spectroscopy exception — and the premise is false in the corpus: the PV360 PRESS_1H scans are spectroscopic with PVM_EncNReceivers = 4.

channels feeds block_size and acq_length, so a wrong value is a wrong per-scan block length.

The first branch was also dead code. #NAME.accessor expands to self['NAME'].accessor:

$ python -c "from brukerapi.dataset import Dataset; print(Dataset._sub_parameters(None, \"#ACQ_dim_desc.__class__.__name__=='str'\"))"
self['ACQ_dim_desc'].__class__.__name__=='str'

which is the Parameter object's class, always 'GenericParameter'. Only the array-valued branch could fire, i.e. CSI.

Change

Read PVM_EncNReceivers unconditionally — spec 3.1 names it the method-side mirror of the Yes count in ACQ_ReceiverSelect — and fall back to one channel where the method declares none.

No fid in the corpus changes: all 204 keep their channel count, block size and shape.

Scope

This does not by itself make a multi-receiver CSI fid readable — the CSI encoding_space/k_space recipes carry no receiver axis either. With the count right it now fails the storage size check loudly instead of folding the channels into the spectral axis, and no dataset available to the project exercises that layout, so inventing one would be guesswork.

Test

test_a_spectroscopic_acquisition_keeps_its_receiver_count — a synthetic two-receiver CSI experiment (ACQ_dim_desc = Spectroscopic Spatial Spatial, the shape that actually triggered the hard-code). Fails on master.

Suite: 2141 passed, 12 skipped.

gdevenyi and others added 2 commits August 8, 2026 16:33
`ruff format --check` reported twelve files as unformatted, so any change
touching one of them dragged unrelated reflow into its diff. Run the
formatter once, on its own, so subsequent changes show only what they
actually change.

Formatting only: no behaviour changes, and `ruff check` is clean before and
after. The suite is unchanged at 2140 passed, 12 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
…im_desc

The channels recipe asserted Nchan == 1 whenever ACQ_dim_desc[0] was
Spectroscopic. ACQ_dim_desc describes a dimension as Spatial or
Spectroscopic (spec 5.1) and says nothing about receivers, and spec 3.1
defines Nchan as the number of active receiver channels with no spectroscopy
exception. The premise is simply false: the PV360 PRESS_1H scans are
spectroscopic with PVM_EncNReceivers = 4.

channels feeds block_size and acq_length, so a wrong value is a wrong
per-scan block length -- short by the receiver factor for a multi-receiver
CSI fid.

The first of the two hard-coding branches was also dead. #NAME.accessor
expands to self['NAME'].accessor, so #ACQ_dim_desc.__class__.__name__ is the
Parameter object's class, always 'GenericParameter', never 'str'. Only the
array-valued branch could fire, i.e. CSI.

Read PVM_EncNReceivers unconditionally -- spec 3.1 names it the method-side
mirror of the Yes count in ACQ_ReceiverSelect -- and fall back to one channel
where the method declares none. No fid in the corpus changes: all 204 keep
their channel count, block size and shape.

Note this does not by itself make a multi-receiver CSI fid readable: the CSI
encoding_space and k_space recipes carry no receiver axis either. With the
count right it now fails the storage size check loudly instead of reading the
channels into the spectral axis, and no dataset available to the project
exercises that layout.

Closes isi-nmr#202

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
@gdevenyi

gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto #213 (ruff format), so this branch carries that commit until #213 merges — merge #213 first and this diff reduces to its own change. Verified after the rebase: ruff check and ruff format --check clean, full suite green.

Integration check across all eleven conformance PRs merged together: 2164 passed, 12 skipped; corpus load test 3202 → 3207 of 3478 with zero newly broken datasets.

@headmeister
headmeister merged commit b237669 into isi-nmr:master Aug 10, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fid channel count is hard-coded to 1 for spectroscopic ACQ_dim_desc, and the first channels condition is dead code (spec 3.1, 5.1)

2 participants