dcm2niix v1.0.20260724 - #1026
Merged
Merged
Conversation
XA30/XA60 scanners (syngo MR XA*) export PMU recordings as Raw Data Storage DICOMs whose private tag (7FE1,1010) carries a gzip-compressed XML payload (Siemens PhysioLogging feature): https://www.magnetomworld.siemens-healthineers.com/clinical-corner/application-tips/physiologging These were previously matched by the existing RawDataStorage detection and skipped with no output. This commit recognises the gzip-XML PMU form, decompresses it, and writes BIDS-compliant physio sidecars (`<base>_recording-<label>_physio.tsv.gz` plus matching `.json`) alongside the rest of the conversion output. Implementation -------------- * nii_dicom.h: TDICOMdata gains `isXAPhysio`, `xaPhysioOffset`, `xaPhysioBytes` for the detected payload. * nii_dicom.cpp: new tag macro `kSiemensXAPhysio` (7FE1,1010) and a parser case that fires only on Raw Data Storage SOPs and only when the payload begins with the gzip magic (1F 8B). Marks the file `isValid = true` so it survives the dispatcher's image filter and reaches the new XA hook. * nii_dicom_batch.cpp: short-circuit at the top of saveDcm2NiiCore runs `xaPhysioConvert()` for XA physio files and returns before any NIfTI machinery executes. The `_Raw` filename suffix is suppressed on the local TDICOMdata copy so the BIDS prefix is clean. XA conversion details --------------------- * Decompression uses miniz with raw inflate after manually parsing the 10-byte gzip header. The gzip ISIZE trailer is intentionally not trusted because DICOM OB padding can append bytes after the deflate stream end; output buffer is sized at 16x the compressed payload (capped at 32 MB) and total_out is the truth at STREAM_END. * DOCTYPE blocks are stripped before XML scanning to neutralise internal-entity / billion-laughs payloads, mirroring the bidsphysio Python parser. * XML extraction is hand-rolled (no XML dep): a strstr-based scanner walks `<PhysioStream TYPE="...">` blocks, pulls `<PMU TIME_TICS="..." DATA="...">` rows, and reads the volume timeline from `<Volume ACQUISITION_TIME_TICS="...">`. * Per stream: sample interval is span/(N-1) ms (fencepost-correct equivalent of LogStartMDHTime/LogStopMDHTime), StartTime is the first PMU tic minus the first volume tic in seconds (typically negative because PMU recording starts manually before the scan), and the BIDS trigger column is rasterised by snapping each volume tic to its nearest PMU sample. Stream type maps to BIDS labels (PULS->cardiac, RESP->respiratory, ECG->ecg, EXT->external_trigger) so output is interchangeable with bidsphysio's. Output for the bidsphysio reference fixture matches the cbinyu/ bidsphysio Python converter sample-for-sample: 4480 cardiac samples at 200 Hz, 1120 respiratory samples at 50 Hz, StartTime=-2.1475 s, 10 volume triggers per stream. Verified -------- * dcm_qa, dcm_qa_nih, dcm_qa_uih all pass with no diffs (legacy paths untouched). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This builds on the prior XA60 PhysioLogging commit (e603666) by adding detection and conversion of legacy CMRR Multi-Band (VE11C) PMU DICOMs that share the same private tag (7FE1,1010) on the Siemens CSA Non-Image Storage SOP. The two formats now emit byte-equivalent BIDS output via a shared helper layer, matching the Python reference parser bidsphysio sample-for-sample. Format coverage --------------- At (7FE1,1010), the parser distinguishes: * XA gzip-XML (XA30/XA60 PhysioLogging) — gzip magic 1F 8B * CMRR VE11C raw binary blob (legacy) — 1024-byte waveform headers + ASCII log lines, validated by the fname matching one of `_PULS.log`/`_RESP.log`/`_EXT.log`/`_ECG.log`/`_Info.log` so MR Spectroscopy DICOMs (which share the same SOP) are not misclassified. The Siemens CSA Non-Image SOP `1.3.12.2.1107.5.9.1` is now recognised by the existing isRawDataStorage detection (it is also used for MRS, hence the fname-based disambiguation in the CMRR detector). Shared helpers (physio* in nii_dicom_batch.cpp) ----------------------------------------------- Both converters funnel through a small layer that bidsphysio compatibility relies on: * physioBidsSortByTic() — defensive stable sort by tic (XA + CMRR). * physioBidsFillUniform() — rebuilds (signal, trigger) on the uniform timeline implied by the sample rate, NaN-filling missing slots (sparse streams like EXT) and rasterising volume tics with ceiling semantics to match bidsphysio's `argmax(times >= t)`. * physioBidsEmitStream() — wraps the uniform-fill, ms-truncated StartTime computation (matching bidsphysio's `int(t_start_ms)/1000` rule), and write step. * xaPhysioWriteStreamFiles() — emits `<base>_recording-<label>_physio .tsv.gz` plus the BIDS JSON sidecar; NaN samples are written as the literal string "nan" to match bidsphysio. Round-4 audit hardening ----------------------- * cmrrPhysioAppend() commits each realloc result to the struct before attempting the next, eliminating a dangling-pointer hazard if the second realloc fails. * The `acquNum * 1024` waveform-stride calc bounds acquNum against INT_MAX/1024 before the multiply. * The CMRR header-validation guard tightened from `lLength >= 1024` to `> 1024` so the body-byte sniff at offset 1024 cannot read one past the DICOM element value. * Output-cap-vs-stream-end disambiguation in xaPhysioInflate's Z_BUF_ERROR branch now warns when the user's payload exceeds the 32 MB internal cap. * Modal-tic divergence warning uses np.unique-style memory (not bincount, which would allocate O(max(diff))). * PMU samples are stable-sorted; negative TIME_TICS rejected; non-positive dt skip-with-warning. * DOCTYPE strip handles internal-subset, external-DTD, and bare DOCTYPE forms — no count cap, so multi-DOCTYPE bypass attempts fail closed. * NaN test uses isnan() from <math.h> (added <limits.h> for INT_MAX). Output equivalence with bidsphysio (verified) --------------------------------------------- For the canonical bidsphysio fixtures (samplePhysioXA60.dcm and samplePhysioCMRR.dcm) all streams produce identical row counts, signal values, and trigger placements; JSON sidecars are byte-identical for PULS / RESP / cardiac / respiratory streams. The single diverging case is the EXT StartTime on sparse streams: bidsphysio emits -0.024 due to floating-point precision loss in its tics→seconds→ms→int chain, while dcm2niix stays in the integer-tic domain until the final division and emits the mathematically correct -0.025. Both differ by 1 ms. Verified -------- * dcm_qa, dcm_qa_nih, dcm_qa_uih all pass with no diffs (legacy image paths untouched). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reimplements heudiconv's reproin heuristic in C so dcm2niix can emit BIDS-style filenames in a single pass, then ships a stdlib-only Python helper (tools/reproinx.py) that walks the output tree to cover the cross-series concerns one-pass conversion cannot resolve. See REPROIN.md for the grammar, precedence rules, defaults, privacy notes, and the full list of limitations the post-pass is and is not responsible for. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CLAUDE.md drops the now-mature PET/BIDS and Siemens physio sections plus the orphaned feature-macros list, and compresses the ReproIn subsection to defer detail to REPROIN.md. Adds a Pre-push checks subsection covering the dcm_qa regression suite and codespell. .codespellrc whitelists 'puls' so Siemens' PULS waveform/log naming (e.g. _PULS.log) stops being flagged as a typo for PULSE/PLUS. REPROIN.md 're-use' becomes 'reuse' to match codespell's preferred form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
By default, `-f %H` (reproin) appends a project subdirectory under
`-o`, derived from (0008,1030) StudyDescription with
PerformedProcedureStepDescription as fallback. That helps a single
`-o` host multiple studies, but it nests the BIDS dataset one level
deeper than callers who pre-create a named destination directory
(e.g. a GUI wrapper that asks the user to name the new dataset
before invoking dcm2niix) actually want.
`-br <name>` overrides that auto-derivation:
-br MyStudy write to <-o>/MyStudy/sub-XX/... (override the
study-description path with an explicit name)
-br . suppress the project subdirectory entirely, so
<-o> itself is the BIDS root and dataset_description.json
lives directly under the caller's chosen directory
Without `-br`, behaviour is unchanged. Implementation: a new
`isBidsRoot` boolean + `bidsRoot[kOptsStr]` buffer in TDCMopts,
zero-initialized in setDefaultOpts; the reproin emit path in
nii_dicom_batch.cpp branches on isBidsRoot and uses bidsRoot
verbatim (which can be the empty string when the caller passed
`.`) rather than calling reproinBuildStudyPath.
REPROIN.md documents the flag alongside `-bi` / `-bv`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit f6b6cda added an early-return guard to sliceTimingCore that bailed out when hdr->dim[3] > kMaxEPI3D (1024). The intent was to skip slice-timing arithmetic on oversized volumes, but the guard also skipped the call to headerDcm2Nii2 that computes sliceDir and finalises the sform/qform. Descending-Z volumes were therefore no longer flipped, leaving a stale incorrect sform: e.g. a 1252-slice CT abdomen produced srow_z=(0,0,0.75,1240) instead of (0,0,0.5,614.5). Split the guard so only the slice-timing helpers are skipped on oversized volumes; orientation is now computed regardless of slice count. Each slice-timing helper already guards its own kMaxEPI3D writes, so this is safe. See #1015 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dcm2niix's %H one-pass parser now appends a row per converted series to
<study>/.reproin_provenance.tsv (StudyInstanceUID, SeriesNumber,
ProtocolName, SeriesDescription, StudyDescription, OutputStem). reproinx.py
consumes this to apply study-wide fixups the one-pass parser cannot do:
- propagate _ses-X from any series (typically the scout) to every
non-derivative file in the same study
- rename dcm2niix's a/b/c collision suffix to heudiconv-style __dup-NN
ordered by ascending SeriesNumber
- --no-derivatives flag drops the derivatives/ subtree after all other
passes have run
reproinx.py also drops "-w 1" from its dcm2niix invocation so the default
ADD_SUFFIX preserves colliding series instead of silently overwriting them.
Other fixes in nii_dicom_batch.cpp:
- JSON "SequenceName" falls back to PulseSequenceName (0018,9005) when
(0018,0024) is absent (XA60 fMRI; BIDS validator warning).
- PartialFourier is suppressed when pf == 1.0 (full Fourier is the
acquisition default and reporting it adds noise).
reproin.cpp exposes reproinSanitizeProjectPath() so the new provenance
writer can share the same path-safety pipeline as reproinBuildStudyPath.
Docs (CLAUDE.md, README.md, REPROIN.md) updated to describe the post-pass
scope, the high-slice-orientation regression note, and the new flags.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Major changes this commit: * Fix issue #1013 — JPEG Lossless multi-fragment regression. The new per-frame `dti4D->offsetTable[]` loop in `nii_loadImgXL` reads from a `TDTI4D` that is not always the one the parser filled (the `*dti4Ds = *dti4D` copy in `saveDcm2Nii` produces a stale snapshot). For `kCompressC3` (JPEG Lossless 1.2.840.10008.1.2.4.7x) the legacy `decode_JPEG_SOF_0XC3_stack` walks the file directly for SOI markers and works reliably, so the wrapper short-circuits to it. The underlying dti4D contamination still affects other multi-fragment codecs (JPEG-LS, JPEG2000) — tracked separately. * AcquisitionTime / AcquisitionDateTime seconds: `%02.6f` → `%09.6f`. The old format set minimum total width (always exceeded), leaving e.g. `11:58:6.647500`; `%09.6f` zero-pads the integer part to `06.647500`. Fixes BIDS validator `ACQTIME_FMT`. * New `-ba o` anonymisation mode: strips patient PII (PatientName/ID/ BirthDate/Sex/Age/Size/Weight, AccessionNumber, ReferringPhysicianName) but keeps AcquisitionDateTime. Implemented as an independent `isOmitPiiBIDS` flag alongside the existing `isAnonymizeBIDS`; FreeSurfer wrapper accepts `ba=o` symmetrically. Help text updated. * `createDummyBidsBoilerplate` now accepts `taskName` / `acqName` and writes `task-<X>[_acq-<Y>]_bold.json` with the actual entity set of the bold file. Prevents BIDS validator v2 MULTIPLE_INHERITABLE_FILES errors when a series has `_acq-` and the legacy `task-rest_bold.json` stub would otherwise conflict. * SequenceName fallback for Siemens XA60 fMRI. XA60 leaves `(0018,0024) SequenceName` empty but populates `(0018,9005) PulseSequenceName`; the JSON sidecar now promotes the latter into the BIDS-recommended `SequenceName` slot when the former is empty. * `SpoilingState: false` when `(0018,9016) Spoiling = NONE`, paired with the existing positive-spoiling branch that also emits `SpoilingType`. * New `.reproin_provenance.tsv` written by `reproinAppendProvenance` on every series under `-f %H`. Columns gated on `-ba` mode: 6-col under `-ba y` (no demographics), 10-col under `-ba o`/`-ba n` (adds PatientAge, PatientSex, StudyDate, StudyTime — minimum needed for participants.tsv + chronological subject ordering). Schema- mismatch detection rotates stale files to `.bak`; full anon mode removes them outright. Never carries PatientName, BirthDate, AccessionNumber, OperatorsName, ReferringPhysicianName. * `tools/reproinx.py` overhauled to clone heudiconv reproin's output byte-for-byte at the scaffolding layer: - `participants.tsv` columns `participant_id, age, sex, group` — age/sex pulled from provenance, subjects ordered by StudyDate. Non-default headers (curated columns, CRLF) preserved across re-runs. - `scans.tsv` columns `filename, acq_time, operator, randstr` with CRLF line endings (heudiconv default). `randstr` is md5 of Study+Series UIDs; `operator` defaults to `n/a` (privacy). - `CHANGES`, `README`, `.bidsignore` (`.duecredit.p`), `participants.json`, `dataset_description.json`, `scans.json` verbatim from heudiconv. `_save_json` writes no trailing newline. Existing dcm2niix dummy `dataset_description.json` is upgraded. - `--keep-derivatives` flag (default off): `derivatives/scanner/` only is removed after session detection consumes the scout. Curated `derivatives/fmriprep/`, `derivatives/freesurfer/` etc. survive. Default flipped: previously deletion was opt-in. - `__dup-NN` rename groups require matching StudyInstanceUID + ProtocolName + SeriesDescription. Idempotent on re-runs; refuses to overwrite an existing target. - Session backfill filtered to current subject + current study. - Per-series `TaskName` injected so removing the bare `task-X_bold.json` stub (when `_acq-` variants exist) doesn't leave bold files without a TaskName. - `--anonymize` now upgrades the inner `dcm2niix` call to `-ba y` (was `-ba n`); default is `-ba o`. * C hazards swept along the touched ReproIn paths: bounds-checked `snprintf` and full `if (fp != NULL)` guards in `createDummyBidsBoilerplate`; capacity checks before `strcat(pth, studyPth)` in the ReproIn output-path append; `fprintf(fp, "%s", literal)` replaces non-literal format strings. `dcm2niix_fswrapper::__setDcm2niixOpts` now tokenises a `malloc`'d copy with `strtok_r`, frees it on every exit, and skips malformed `key=value` options instead of advancing a NULL pointer. * Docs: `CLAUDE.md` and `REPROIN.md` describe the new TSV, modes, invariants (kCompressC3 gate, %09.6f, SequenceName fallback), and the docs-must-stay-non-destructive contract. `audit_*.md` added to `.gitignore`. `cpfiles.command` (stale macOS helper hardcoding `/Users/rorden/...`) removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- MatrixCoilMode: emit "None" on the Siemens CSA path when patMode is not SENSE/GRAPPA (e.g. pure SMS XA60, CompressedSense) and when the CSA acceleration block is unreadable. Resolves the BIDS validator recommendation for SMS-only XA60 scans; zero in-tree Siemens Ref/ drift verified. - PulseSequenceType: new heuristic from ScanningSequence / SequenceVariant / multiBandFactor (EPI, MPRAGE, Spoiled Gradient Echo, etc.). isSP anchored at all three legal SP token positions to avoid OSP false-positive and to catch leading-position SP. Opt-out: #define myDisablePulseSequenceType. - .gitignore: /temp/ session scratch (defence in depth; gitignore is not a privacy fix). - CLAUDE.md: subsections covering both new fields, the temp/ retention rule, known warts (patMode==256, MPRAGE overgeneralization, UIH/non-Siemens limitations), and explicit "do not tidy" warnings for future contributors. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Note PRs target the development branch, not master. - Document the three -ba modes (y/n/o), including the new "o" omit-PII mode that reproinx.py invokes so _scans.tsv aggregation keeps timestamps. - Surface the reproinx.py companion flags (--anonymize, --strict, --keep-derivatives, --no-convert) so users can find them without reading REPROIN.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- tools/reproinx.py: dataset_description.json now carries DatasetType
"raw", GeneratedBy (single dcm2niix entry with version discovered
from per-series sidecars), and SourceDatasets []. The explicit
"raw" is mandatory: the validator's context.ts infers "derivative"
whenever GeneratedBy is present and DatasetType is absent, which
cascaded ~36 spurious errors (SkullStripped on anat, etc.). Hand-
edited files are preserved and only backfilled when the three
recommended keys are missing.
- tools/reproinx.py: version discovery is deterministic (sorted
candidates), skips derivatives/, prefers sidecars with
ConversionSoftware == "dcm2niix", and is evaluated lazily so
curated files do not pay a recursive JSON scan. The Version key
is omitted (rather than "unknown") when no sidecar carries it.
- nii_dicom_batch.cpp: ParallelReductionFactor{In,OutOf}Plane gate
widened from > 1.0 to >= 1.0 so a value of 1.0 from a real source
(DICOM 0018,9069 / 0018,9155, GE ASSET, Siemens CSA, Philips
PhaseSlice) is reported. Default 0.0 sentinel still suppresses
emission on non-MR / non-parsed paths.
- nii_dicom_batch.cpp: SpoilingState SP-token check now matches the
PulseSequenceType isSP three-clause form (leading "SP\\", sole
"SP", mid-or-trailing "\\SP"). The two sites no longer disagree
on leading-position SP.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- console/nii_dicom_batch.cpp: emit "InstitutionalDepartmentName": "None" when DICOM tag (0008,1040) is empty. Siemens XA60 leaves the tag empty, which trips the BIDS validator's recommendation; "None" is honest absence (matching the MatrixCoilMode "None" convention). - BIDS/README.md: footnote documents the new fallback. Pre-existing table errors corrected while in the file: Modality DICOM tag fixed to 0008,0060 (was wrongly 0008,1060 = Name of Physician Reading Study); removed two duplicate rows for DeviceSerialNumber and StationName. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previously dcm2niix exited 0 when a CLI option was bad or missing its value (e.g. trailing `-d`, `--badflag`, `-b q`), which masked errors in scripts wrapping the binary. - console/nii_dicom.h: new kEXIT_INVALID_PARAM = 12. - console/main_console.cpp: all 19 invalidParam() call sites now return kEXIT_INVALID_PARAM instead of 0; the catch-all else for unknown options returns kEXIT_INVALID_PARAM and bounds-checks argv[i+1] (the previous read past argv[argc] was latent UB that surfaced as the literal "(null)" in the error message). - console/main_console.cpp: `-h` now returns EXIT_SUCCESS immediately after showHelp() instead of falling through to the example- filename block. Help still exits 0 per Unix convention; the fall-through was a separate cosmetic bug. Also bumps kDCMdate to v1.0.20260527. See #1020 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DICOM encapsulated transfer syntaxes (JPEG Lossless, JPEG2000) may
split a single compressed frame across multiple (FFFE,E000) Item
fragments. Previously the decoders started from imageStart and read
only the first fragment's payload, so multi-fragment single-frame
files failed to decode.
Reassembly is shared and codec-agnostic. Per-codec wrappers are kept
small so each decoder owns the call site:
- console/dicom_fragments.{cpp,h} (new): reassembleEncapsulatedFragments()
walks the file in two passes (count + bound-check, then copy) and
returns a malloc'd contiguous codec bitstream. Returns NULL on
single-fragment or error so callers fall back to the pre-1017 path
unchanged. Length math is overflow- and file-length-bounded.
- console/jpg_0XC3.{cpp,h}: decode_JPEG_SOF_0XC3() refactored into a
static buffer-based _core, the original file-based public entry,
and a new decode_JPEG_SOF_0XC3_mem() for the reassembly path.
- console/nii_dicom.cpp (nii_loadImgJPEGC3, nii_loadImgCoreOpenJPEG):
call the helper, decode from the reassembled buffer when non-NULL,
otherwise behave exactly as before. Parser gate at the encapsulation
branch admits multi-fragment ONLY when numberOfFrames <= 1 and the
scheme is kCompressC3 or kCompressJP2K; multi-frame with multiple
fragments per frame still errors out (the helper concatenates ALL
following fragments and cannot recover per-frame boundaries).
- nii_loadImgXL JPEG2000 path now short-circuits to the core when
frames<=1 so the per-frame offset-table loop above the gate does
not read only the first fragment.
Rename kCompressYes -> kCompressJP2K throughout: the old name was used
ambiguously for both "JPEG2000 transfer syntax" (the compressionScheme
tag) and "decompression enabled" (the runtime flag). kCompressJP2K is
strictly the scheme; compressFlag remains the runtime toggle. The
1.2.840.10008.1.2.5 (DICOM RLE Lossless) branch correctly remains
kCompressRLE - flagged inline in nii_dicom.cpp to prevent a future
rename from sweeping it again. A new isAnyJP2K warning gate in
nii_loadDirCore emits a one-shot "Unsupported JPEG2000 transfer
syntax" when a build without OpenJPEG encounters a JP2K dataset.
CLAUDE.md: documents the new gate, the C3 / JP2K wrapper layout, the
rename trap, and adds a source-list fanout warning (every new .cpp in
the dcm2niix call graph must be wired into all five build surfaces:
CMakeLists.txt, makefile, windows.bat, notarize.sh, COMPILE.md - all
five are updated in this commit).
kDCMdate bumped to v1.0.20260603. Passes dcm_qa_frag (the regression
suite that exercises this code path) clean; remaining dcm_validate
matrix diffs are all expected sidecar deltas from prior BIDS
compliance commits.
See #1017
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
bidsguess.py is a thin wrapper around reproinx.py that swaps -f %H for
-f %h (legacy hazardous BIDS naming) and applies an extra cleanup pass
so the dataset passes bids-validator without manual intervention. Both
paths share one Python code base; --bidsguess on reproinx.py is the
toggle, bidsguess.py just injects it.
C side (-f %h fallbacks):
- console/nii_dicom_batch.cpp: when -bi is omitted and PatientID is
non-empty, derive the subject via reproinFixupSubjectId (heudiconv-
style lowercase + strip '-'/'_'); else fall back to "1".
- When -bv is omitted and StudyDate+StudyTime are present, derive the
session as "YYYYMMDDTHHMMSS". The 'T' separator (ISO 8601 compact)
keeps the label alphanumeric — BIDS forbids '-'/'_' in session
labels (bids-validator error code 63).
- Both fallbacks reuse the existing reproin helpers
(reproinFixupSubjectId, reproinSanitizeLabel) so behaviour stays
consistent with -f %H.
Siemens XA localizer detection:
- console/nii_dicom.cpp: new (0021,103F) AutoAlignData branch flags
scouts whose private-tag content contains "Localizer" (case-
insensitive via in-place toupper + plain strstr, matching the
existing privateCreator idiom). Observed value "Head_Localizer" on
XA60; case may vary across XA10/XA30/XA60 firmware.
- The kCodeMeaning (0008,0104) localizer detector is intentionally
commented out — the same tag appears in many other SQ contexts
(RadiopharmaceuticalInformationSequence, AnatomicRegionSequence,
ReferencedImageSequence, ...) and produces false positives on
diagnostic images that merely reference a localizer.
- Localizers are now exempt from the issue-742 partial-volume warning
(ICEdims mismatch is expected for scouts).
Python tooling (tools/reproinx.py + tools/bidsguess.py):
- New --bidsguess flag on reproinx.main; selects -f %h and adds a
pre-pass before the generic reproinx passes:
* Remove discard/ subdirs (BIDS has no discard datatype).
* Demote 3D *_bold -> *_sbref (BIDS BOLD must be 4D).
* Mark single-volume *_dwi.* artifacts for .bidsignore (DWI must be
>=2 vols with bvec/bval).
* Mark dcm2niix's a/b/c collision-suffix files for .bidsignore.
- _write_scans_tsv now consults <bids_root>/.bidsignore and skips
matching files, preventing SCANS_FILENAME_NOT_MATCH_DATASET. Benefits
-f %H mode too.
- _run_dcm2niix now tolerates kEXIT_SOME_OK_SOME_BAD (8) and
kEXIT_INCOMPLETE_VOLUMES_FOUND (10) so partial-conversion datasets
reach the post-pass. Benefits -f %H mode too.
- tools/bidsguess.py forwards every CLI arg to reproinx.main() with
--bidsguess appended unconditionally.
End-to-end on a real multi-vendor Siemens XA60 dataset
(/Users/chris/src/old/DICOM): bids-validator 1.15.0 reports 0 errors
post-bidsguess (was 30+ session-label, 5 BOLD_NOT_4D, 2
VOLUME_COUNT_MISMATCH, 50+ NOT_INCLUDED). The remaining 3 warnings
(INCONSISTENT_PARAMETERS, MISSING_SESSION, README_FILE_SMALL) are
dataset-reality, not wrapper bugs.
kDCMdate bumped to v1.0.20260604.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dcm2niix -f %H routes any series whose protocol does not parse as ReproIn to <bids_root>/Unknown/. Those files were previously orphaned — heudiconv-conformant Reproin-discipline sites are rare in practice, so the Unknown/ pile dominated most real datasets. reproinx.py now rescues them into proper sub-X/ses-Y/<datatype>/ paths using the BidsGuess JSON sidecar field and per-series provenance. C side (console/nii_dicom_batch.cpp): - .reproin_provenance.tsv gains an 11th column, PatientID, in default -ba o / -ba n modes. Withheld in -ba y so anonymise stays clean. - expectedTabs raised 9 -> 10 in the schema-mismatch peek; any pre-existing 10-col TSV from an older build auto-rotates to .bak on first append (existing logic, repurposed). The 6-col -ba y schema is unchanged. Python side (tools/reproinx.py): - New _rescue_unknown_dir(): for each *.json in <bids_root>/Unknown/, reads BidsGuess + the matching provenance row (keyed on StudyInstanceUID + SeriesNumber), derives the target via reproinFixupSubjectId-equivalent on PatientID and YYYYMMDDTHHMMSS from StudyDate+StudyTime, and moves the whole file family (.nii / .nii.gz / .json / .bvec / .bval) together. Collisions get _run-NN appended. - func/_bold and func/_sbref REQUIRE _task-X_ per BIDS. The C BidsGuess drops task from its entity suffix; the rescue recovers it from ProtocolName (fallback SeriesDescription) via task-([A-Za-z0-9]+), defaulting to "rest" — matches the C %h createDummyBidsBoilerplate fallback. - Generic hygiene pass (3D _bold -> _sbref, single-volume DWI -> .bidsignore, a/b/c collision-suffix files -> .bidsignore, residual Unknown/ files -> .bidsignore) now runs unconditionally instead of being gated on --bidsguess. Retirement: - tools/bidsguess.py deleted. - --bidsguess flag and the `bidsguess` parameter on _run_dcm2niix removed; reproinx.py always invokes dcm2niix with -f %H. - The C-side -f %h fallbacks (PatientID + datetime defaults from the previous commit) are kept — they're useful for anyone invoking dcm2niix directly without the wrapper. - Rationale: the Unknown/-rescue makes default reproinx.py produce the same validator-clean output bidsguess.py was producing. The one edge case --bidsguess covered (a study with a mix of ReproIn-named and ad-hoc series, where forcing PatientID-based subject consistency beats honouring partial ReproIn naming) is niche; if it bites someone, a future smaller-scoped flag is the right fix, not a whole alternate filename mode. Verified on /Users/chris/src/old/DICOM (3 subjects, mixed naming): 24 file-stems rescued, bids-validator 1.15.0 reports 0 errors and only MISSING_SESSION / README_FILE_SMALL warnings (dataset-reality, not wrapper bugs). CLAUDE.md updated to reflect the 11-column provenance schema. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…on loop Audit found one real security issue and one quality nit in _rescue_unknown_dir; both addressed here. Path-traversal hardening (high-severity): - _session_token_from_studydatetime now validates StudyDate as 8 digits (DICOM VR DA) and StudyTime as 6+ digits (VR TM) before emitting "YYYYMMDDTHHMMSS". A malicious DICOM with e.g. StudyDate="../../etc" previously flowed through the C-side reproinTsvField (which strips only \t\r\n) into the TSV, into _rescue_unknown_dir, and into Path.rename. Invalid input now returns "" and the file stays in Unknown/. - _rescue_unknown_dir now matches BidsGuess[0] against _BIDS_DATATYPES (allowlist of BIDS top-level datatype dirs) and BidsGuess[1] against [A-Za-z0-9_-]* before either reaches a Path component. Failures route the file to Unknown/ where the existing .bidsignore sweep catches it — no data loss, no traversal. Collision-loop tidy (refactor): - Replace the ad-hoc `run_idx += 1 if run_idx else 2` (which silently skipped _run-01) with a clean `for idx in range(1, 100)` starting at _run-01. The un-numbered first arrival keeps its plain stem; only second-and-later collisions get _run-NN. - Precompute the run-NN template via rpartition once instead of rebuilding it on every iteration. - Helper _has_family() makes the family-existence test obvious. Smoke-tested against /Users/chris/src/old/DICOM: 24 file-stems rescued, bids-validator 1.15.0 reports 0 errors and only MISSING_SESSION / README_FILE_SMALL warnings (unchanged from the pre-fix run). Deferred audit items documented in CLAUDE.md: - Partial-state rename loop (accepted because src+dst are on the same FS and target_dir.mkdir would already have failed first). - _load_provenance silently coalesces duplicate header columns (not user-reachable; only the C side writes the TSV). - 32-bit overflow window in reassembleEncapsulatedFragments (narrow: requires 32-bit build AND >2GB encapsulated frame). - _bidsguess_* helper-name rename (cosmetic; helpers are private). - Shared _load_file_to_buf helper between jpg_0XC3 and dicom_fragments (cross-file refactor; deferred for an issue-1017 follow-up). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dcm2niix -f %H writes to <out>/<StudyDescription>/... and reproinPathify in console/reproin.cpp splits spaces and underscores in StudyDescription into path separators. That's deliberate for reproin sites: StudyDescription="Smith_Aging" yields the useful two-level <out>/Smith/Aging/sub-X/ grouping that mirrors the heudiconv reproin <locator>/<study> convention (see reference output in /Users/chris/src/reproin/reproout_*). For non-reproin datasets the same machinery produces content-free filler — StudyDescription="Studyname Studyname" → Studyname/Studyname/ buried below outdir. The new _maybe_collapse_nonreproin_root pre-pass detects this and collapses the redundant hierarchy into out_root. Detection: walk the provenance TSV's OutputStem column. The one-pass ReproIn writer's prefix is `sub-`. Rows under `Unknown/` mean reproin failed for that series; rows under `derivatives/scanner/` mean dcm2niix routed a scout / DERIVED-flagged file regardless of reproin parsing. If NO row starts with `sub-`, no series benefited from the hierarchy and the contents are safely promoted to out_root. Safety: - Only ONE provenance TSV may exist below out_root (multi-study trees keep their grouping). - Every directory from out_root down to the study_root must be single-child (avoids clobbering sibling content). - Pre-flight clobber check: refuse if any study_root child name collides with an existing entry in out_root. Runs before all other passes so subsequent rglob-based discovery (rescue, scans.tsv, scaffolding) sees the final paths. README/dataset_description.json now land directly at out_root for non-reproin trees while reproin trees keep their <locator>/<study> hierarchy untouched. Verified end-to-end on the non-reproin /Users/chris/src/old/DICOM dataset (StudyDescription = "Studyname Studyname"): before: /tmp/bidsH/Studyname/Studyname/sub-ol0001/... after: /tmp/bidsH/sub-ol0001/... bids-validator 1.15.0: 0 errors, only MISSING_SESSION / README_FILE_SMALL warnings (unchanged from pre-collapse). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ss prefix) Three small fixes from the post-9a1b337 audit pass; the higher-severity items were either intentional (CT in Unknown/ is more validator-clean than promoting it into a non-spec `ct/` datatype dir) or out of scope for a single-user trusted-input tool (TOCTOU/symlink races on the collapse). Both decisions documented in CLAUDE.md so the next audit cycle doesn't re-litigate them. Code: - _BIDS_ENTITY_SUFFIX_RE: `*` -> `+` so an empty BidsGuess[1] is rejected. Previously would have produced a malformed `sub-X_ses-Y` with no suffix word. No real-world C-side path emits empty here, but the regex is the trust boundary so it should be strict. - Extract _REPROIN_SUCCESS_PREFIX = "sub-" with docstring tying it to the C-side OutputStem contract. Single use site (the collapse detector) now reads against a named constant rather than a literal. - Trim two restate-WHAT comments from _maybe_collapse_nonreproin_root. Docs (CLAUDE.md): - Document the intentional CT exclusion from _BIDS_DATATYPES (BIDS has no `ct` datatype; promoting trips the validator). - Document the trust-boundary stance on TOCTOU/symlink races so the audit-deferred items have a stated rationale. Verified end-to-end: same 0-error / 2-advisory-warning result on the /Users/chris/src/old/DICOM dataset. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds three additive heuristics from BIDS-Manager
(bidsmgr/classifier/sequence_dict.py, MIT-licensed) as a new
vendor-agnostic refinement pass that runs after
setBidsSiemens/Philips/GE inside setBids(). Source for matching is the
lowercased ProtocolName + " " + SeriesDescription.
1. DWI scanner-derivative override. Word-boundary token match on
colfa/expadc/tracew?/tensor/fa/adc/s0map (plus '_'/'-' separator
variants for the compound names). TENSOR routes to dataType=
"derived" so the file lands under derivatives/scanner/ via the
existing setBids return-value gate. Boundary check is load-bearing:
"MPRAGE_FATsat" must NOT match "fa"; "dwi_AP" must NOT match "adc".
2. Task-name hints fallback. Only fires when bidsDataType=="func" AND
bidsTask is empty AND _task- is not already in the suffix. First
tries explicit task-<label> at word boundary; else walks a 12-entry
curated dict (rest, movie, nback, flanker, stroop, motor,
checkerboard, exec, paradigm, sparse, activation, task) with simple
substring match. Replaces the %h hardcoded task-rest fallback with
a more honest task name for non-ReproIn protocols.
3. _acq-X and _dir-{AP,PA,LR,RL} entity extraction. Only fires when
the entity is not already present in the suffix. acq picks the
longest word-boundary match; dir picks the earliest. New
bidsInsertEntity() helper splices entries at canonical BIDS-2
entity order (task/acq/ce/rec/dir/run/echo/flip/inv/part/<suffix>).
New helpers in nii_dicom_batch.cpp (near line 1271 next to other
string utilities):
- bidsStrLower(): lowercase copy
- bidsIsBoundary(): word-boundary char check (NUL/_/-/space/etc.)
- bidsFindTokenBdy(): strstr + word-boundary verification (mirrors
Python's (?:^|[_-])token(?=$|[_-]) regex)
- bidsInsertEntity(): canonical-order splice with no-op-if-present
- setBidsHeuristics(): the four sub-passes above
Vendor-specific code (setBidsSiemens, setBidsPhilips, setBidsGE) is
untouched. The new pass only fills gaps the vendor heuristic left:
DWI override is the only one that can override a vendor decision, and
it requires a word-boundary token in the protocol text. In-tree
regression suite (dcm_qa, dcm_qa_nih, dcm_qa_uih) shows zero new
BidsGuess deltas — every diff is pre-existing BIDS-compliance work
from earlier commits.
Attribution: BIDS-Manager, MIT license, Copyright (c) 2023 BIDS
Manager. CLAUDE.md gains a "BIDS-Manager-derived heuristics"
subsection documenting the policy and the word-boundary correctness
rationale.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When dcm2niix routes a localizer / Phoenix Document / other non-image DICOM that the C side flagged as "discard" into Unknown/ (which can happen under -f %H when reproin parsing doesn't apply), the rescue pass leaves the file there — its BidsGuess[0]=="discard" disqualifies it from being promoted into a sub-X/ses-Y/<datatype>/ path. Previously the .bidsignore sweep then routed it for the validator to skip. That keeps the file but leaves an Unknown/ folder cluttering the BIDS root. New _purge_all_discard_unknown(bids_root, strict) runs immediately after _rescue_unknown_dir on the same root. It deletes the whole Unknown/ folder when: - Every *.json inside has BidsGuess[0] == "discard" - Every non-JSON file has a matching JSON sidecar (no orphans) A single non-discard JSON (e.g. CT, which is deliberately omitted from _BIDS_DATATYPES) preserves the folder so the .bidsignore sweep can still route it. Verified on /Users/chris/src/bidsui/datasets/OldenburgXA30: - "removed all-discard Unknown/" fires; the folder is gone from the collapsed BIDS root. - .bidsignore drops from 14/10 entries (varies by run) to 8 — only the genuine single-volume DWI artifacts remain. - bids-validator: 0 errors, same MISSING_SESSION + README_FILE_SMALL advisory warnings as before. CLAUDE.md gains a paragraph next to the rescue documentation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
EPI-based SWI sequences (e.g. *swi3d_epr on Siemens XA80) and SWI mIP / SWI_Images derived projections did not match any of the existing sequence-name branches (fl3d, gre, ep_seg_fid). They fell through to the trailing `if (isDerived) dataTypeBIDS = "derived"` clobber at the end of setBidsSiemens and ended up routed to derivatives/scanner/. New override fires when ImageType contains the token "SWI": classify as anat / T2starw, set isPart=true (so _part-mag / _part-phase is appended), and clear isDerived so the trailing clobber leaves the "anat" decision intact. Test uses strstr(d->imageType, "_SWI") — the underscore prefix anchors to the DICOM ImageType token boundary (ImageType is underscore-joined) so SWI is matched as a token, not as a substring of SWIRL etc. Verified on /Users/chris/src/dcm_qa_xa80/In/14_t2_ep_swi_tra_2.5mm/ (ImageType ["DERIVED","PRIMARY","SWI","MINIMUM","MAGNITUDE"]): before: ["derived","_acq-swi3_run-14_part-mag_mIP"] after: ["anat","_acq-swi3_run-14_part-mag_T2starw"] In-tree regression suite (dcm_qa, dcm_qa_nih, dcm_qa_uih) clean: zero new BidsGuess deltas. The override fires only when ImageType itself contains the SWI token, which none of the regression samples have. The pre-existing ep_seg_fid seriesDescription-based mIP / SWI_Images demote at line ~8031 is retained as a back-compat secondary path for cases where ImageType lacks _SWI but the series description carries those tokens. CLAUDE.md gains a "SWI ImageType override in setBidsSiemens" subsection documenting the override's position and interaction with isDerived. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fl3d_vibe (Siemens VIBE) has no fixed contrast — the same sequence name can be tuned as T1-weighted (high flip angle), PD-weighted (low flip angle), or T2*-weighted (long TE). Until now setBidsSiemens did not have a branch for it, so VIBE series produced no BidsGuess at all. New branch fires on strstr(seqDetails, "fl3d_vibe") and classifies by acquisition physics: - T1 estimate: 0.8 * fieldStrength^0.38 (Bottomley approximation) - T2* estimate: 0.050 / fieldStrength - Ernst angle: acos(exp(-TR/T1)) in degrees Decision: - TE >= 0.5 * T2* -> T2starw - flipAngle >= 1.3 * Ernst -> T1w - flipAngle <= 0.7 * Ernst -> PDw - otherwise -> PDw (mixed structural default) dataType is "anat" and isPart=true so the BIDS suffix carries _part-mag (or _part-phase if applicable). Guarded on positive TR, TE, fieldStrength, and flipAngle — when any are missing the cascade falls through to the trailing `if (isDerived) dataTypeBIDS = "derived"` clobber rather than producing a bogus classification. Verified on /Users/chris/src/dcm_qa_xb10/fx/10_t1_vibe_tra_cs22/ (3T, TR=5.18ms, TE=2.46ms, FA=20 deg): Ernst angle ~= 5.3 deg FA / Ernst ~= 3.8 -> T1w before: (no BidsGuess emitted) after: ["anat","_acq-fldyn3p22_run-10_part-mag_T1w"] In-tree regression suite (dcm_qa, dcm_qa_nih, dcm_qa_uih) shows zero new BidsGuess deltas; the branch only fires when seqDetails contains "fl3d_vibe", which no regression series does. CLAUDE.md gains a "fl3d_vibe Ernst-angle physics classifier" subsection documenting the formulas, decision rules, and field-strength guards. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two related fixes for Siemens SPACE (variable-flip-angle TSE family, the marketing name for tse_vfl on XA-line scanners) being mislabelled as functional BOLD. Root cause: the bold cascade's `pace` check was a bare substring match — strstr(seqDetails, "pace") matched "space" as a substring, so any SPACE sequence whose seqDetails was "%SiemensSeq%\space" fell into the bold branch. The comment "n.b. Space is not pace" was a warning to whoever wrote the check, not an enforcement. Compounding bug: the FLAIR override that should have fired earlier was inside the tse_vfl branch, whose outer condition is strstr(seqDetails, "tse_vfl") — but XA-line SPACE writes "space" in seqDetails (not "tse_vfl") so that branch never fired. And the inner spcir check looked at d->sequenceName, which is empty on XA-line — the actual FLAIR signal lives in d->pulseSequenceName (e.g. "*spcir_220ns"). Fixes (both in nii_dicom_batch.cpp setBidsSiemens): 1. bold branch (~line 8079): "pace" -> "_pace". Siemens PACE sequences are always written ep2d_bold_PACE / ep2d_pace, so anchoring on the underscore is safe. 2. tse_vfl branch (~line 7982): outer condition now also matches strstr(seqDetails, "\\space"). Inner FLAIR check now also reads d->pulseSequenceName for "spcir". Verified on /Users/chris/src/dcm_qa_xb10/flair/in/: before: ["func","_acq-spcir2p2_dir-RL_run-5_bold"] after: ["anat","_acq-spcir2p2_run-5_FLAIR"] In-tree regression suite (dcm_qa, dcm_qa_nih, dcm_qa_uih) shows zero new BidsGuess deltas. The narrowed `_pace` and the new `\space` clause only affect series whose names contain those specific tokens, none of which are in the regression set. CLAUDE.md gains a "Siemens SPACE / FLAIR detection" subsection documenting both fixes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase A of MRS support (Phase B is the conversion pipeline itself). These two DICOM tags are useful for any image — not just MRS — so they land first as a small isolated addition. nii_dicom.h: - kMRSAcqNone / kMRSAcqSingleVoxel / kMRSAcqRow / kMRSAcqPlane / kMRSAcqVolume enum mirroring DICOM CS (0018,9200) values. - TDICOMdata: new int fields mrsAcqType (the enum) and numberOfKSpaceTrajectories. nii_dicom.cpp: - New tag macros kNumberOfKSpaceTrajectories (0018,9093) and kMRSpectroscopyAcquisitionType (0018,9200). - Parser case for both; the (0018,9200) parser maps the DICOM CS string (SINGLE_VOXEL / VOLUME / PLANE / ROW) to the enum so the emission side only deals with the integer code. - clear_dicom_data initialises mrsAcqType=kMRSAcqNone and numberOfKSpaceTrajectories=0 so non-MRS series are silent. nii_dicom_batch.cpp: - JSON sidecar gains "NumberOfKSpaceTrajectories" when > 0 and "MRSpectroscopyAcquisitionType" (string echo of the DICOM CS) when the enum is non-zero. Both are guarded so non-MRS sidecars are unchanged byte-for-byte. In-tree regression (dcm_qa, dcm_qa_nih, dcm_qa_uih) clean — no unexpected diffs and no BidsGuess deltas. The two new fields only fire on MRS DICOMs (none of the regression samples are MRS). Setting up the field plumbing for Phase B (the actual XA SVS DICOM -> NIfTI MRS pipeline ported from spec2nii, BSD-3-Clause). That commit will reference these enum values for SVS vs CSI dispatch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
XA60 Siemens MR Spectroscopy Storage SOP (1.2.840.10008.5.1.4.1.1.4.2)
DICOMs now convert to NIfTI-1 with complex64 image data and the
BIDS-MRS sidecar fields. Output is byte-identical to spec2nii's
reference NIfTI image data (BSD-3-Clause, William Clarke, U. Oxford
2020). SVS only — MRSI / Unloc / mrsref are deferred until sample
data is available.
console/nii_dicom.{cpp,h}:
- New TDICOMdata fields: isMRS, dataPointColumns (0028,9002),
spectralWidth (0018,9052 Hz), resonantNucleus (0018,9100).
TransmitterFrequency reuses imagingFrequency (0018,9098).
- SOP class check at kMediaStorageSOPClassUID flags isMRS for the
MR Spectroscopy Storage UID.
- kSpectroscopyData (5600,0020) now captures imageStart + imageBytes
for the FID block instead of rejecting the file with "Skipping
Spectroscopy DICOM".
- isValid gate at the bottom of readDICOMx admits MRS files whose
spatial dims are 1x1x1 (an SVS voxel has no slice grid).
- Parsers for kSpectralWidth, kResonantNucleus, and
kSpectroscopyAcquisitionDataColumns.
console/nii_dicom_batch.cpp:
- New saveDcm2NiiMRS: reads each FID into a single buffer (stacked
along NIfTI dim[5]), applies the NumarisX (XA) phase convention
(specData[0::2] - 1j*specData[1::2]) while preserving +0.0 in the
imag channel to avoid byte differences vs spec2nii's reference,
builds the affine via spec2nii's dcm_to_nifti_orientation formula
(Q^T * diag([PixelSpacing[1], PixelSpacing[0], SliceThickness])
then LPS->RAS negation of rows 0-1), and emits via the standard
nii_saveNII + nii_SaveBIDSX writers.
- saveDcm2Nii dispatches to saveDcm2NiiMRS when the lead DICOM has
isMRS, so the image-data branch never sees MRS data.
- BidsGuess "mrs"/"_svs" is set before nii_createFilename so the
output filename uses the BIDS suffix.
- JSON sidecar now emits SpectralWidth, DwellTime (= 1/SpectralWidth),
TransmitterFrequency, ResonantNucleus, and
SpectroscopyAcquisitionDataColumns when d.isMRS.
- The pre-existing Siemens-EPI DwellTime emitter is gated `!d.isMRS`
so MRS files don't end up with two DwellTime keys.
CLAUDE.md gains an "MR Spectroscopy (MRS) pipeline" subsection
documenting the SOP dispatch, FID capture, phase convention quirk,
affine formula, JSON fields, and the +0.0/-0.0 byte-identity gate.
Verified end-to-end against /Users/chris/src/spec2nii/XA60/Ref/
(series30 = 64 averages, series31 = 1 average):
- dim, datatype (32 = DT_COMPLEX64), pixdim, sform_code (2) match
(sform values agree to float32 precision; spec2nii uses NIfTI-2)
- image data byte-identical (524288 / 8192 bytes respectively)
- JSON has BidsGuess ["mrs","_svs"], SpectralWidth 2000,
DwellTime 0.0005, TransmitterFrequency 297.155, ResonantNucleus
"1H", MRSpectroscopyAcquisitionType "SINGLE_VOXEL"
In-tree regression suite (dcm_qa, dcm_qa_nih, dcm_qa_uih) clean —
the new code path is gated on isMRS, which no regression sample
triggers.
Attribution: spec2nii (BSD-3-Clause), William Clarke, U. Oxford 2020.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Response to 2026-06-05 external audit (audit_temp.md). Fixes for the high-confidence findings, documentation in CLAUDE.md for the accepted- risk ones, version bump. reproinx.py: - H1: _run_dcm2niix now returns dcm2niix's exit code (was None); main() propagates it so automation can distinguish a clean run from one that lost series via kEXIT_SOME_OK_SOME_BAD (8) or kEXIT_INCOMPLETE_VOLUMES_FOUND (10). - H2: _bidsguess_demote_3d_bold preflights every target extension (.nii / .nii.gz / .json / .bvec / .bval) before renaming so a real *_sbref family can't be clobbered. Skipped demotes log a reproinx: warning. - H3: new _bids_root_for_session(session_dir) helper returns the right BIDS root for both sessioned (sub-X/ses-Y) and sessionless (sub-X) layouts. Replaces both `session_dir.parent.parent` sites that silently walked off the top for sessionless trees. - H5: _BIDSGUESS_COLLISION_RE extended with FA/ADC/colFA/expADC/trace/ S0map/TENSOR (covering commit 2dad442's heuristic outputs) plus svs/mrsi/unloc/mrsref (pre-emptive for Phase C). The C and Python suffix sets are now in sync; the L3 coupling concern is documented. - M5: _write_scans_tsv emits POSIX paths via .as_posix() so Windows- built BIDS trees comply with the spec's forward-slash requirement. nii_dicom_batch.cpp: - H6: %h path now scrubs opts.bidsSubject / opts.bidsSession through reproinSanitizeLabel and bounds the strcat against the local kOptsStr buffer. Empty/fully-stripped input falls back to "1". Same treatment the %H path already gives these CLI values. - M2: short-token task hints (rs/motor/exec/task) now use bidsFindTokenBdy word-boundary matching instead of bare strstr. Long/unique hints (movie/flanker/stroop/paradigm/sparse/...) stay on plain strstr — they're long enough that drift is implausible. Per-entry wordBoundary flag in kTaskHints makes the policy explicit. - L1: reproinAppendProvenance now checks remove()/rename() return values during schema-mismatch rotation. On failure the appender emits printWarning and bails so new-schema rows never land under the old header. nii_dicom.h: - kDCMdate bump to v1.0.20260605. CLAUDE.md: - New "Audit-deferred accepted-risk decisions (2026-06-05 external review)" section persisting the H4 / M1 / M3 / L3 rationale plus M7 deferral so future audits don't re-litigate them. Verified: - cmake build clean. - In-tree regression (dcm_qa, dcm_qa_nih, dcm_qa_uih) clean — zero new BidsGuess deltas, zero unexpected sidecar diffs. - MRS Phase B byte-identity check still passes against /Users/chris/src/spec2nii/XA60/Ref/series30.nii.gz (524288 bytes identical). - tools/reproinx.py parses (ast.parse). audit_response.md written locally (gitignored per project convention) documenting each finding's disposition. audit_temp.md removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Response to the second 2026-06-05 external audit (Phase B MRS-focused). Code fixes for the high-confidence findings; CLAUDE.md gains a section documenting the four architectural / coverage deferrals (M5, M6, L3, L4). console/nii_dicom_batch.cpp: - H3: swapEndian() special-cases DT_COMPLEX64 and swaps each float32 component (2*nVox of them) rather than treating (real,imag) as a single 8-byte scalar. --big-endian y on MRS output is now correct. - H2: saveDcm2NiiMRS() byte-swaps each FID slot via nifti_swap_4bytes when d->isLittleEndian is false. XA-line Siemens always emits Explicit VR Little Endian, but the swap covers Explicit VR Big Endian MRS Storage which is legal per the DICOM spec. - H4: MRS dim/size overflow guards. DataPointColumns > 32767 or N_files > 32767 (NIfTI-1 dim[k] limits) is rejected before allocation; nii_ImgBytes(hdr) == total_bytes is asserted after header construction so future internal accounting drift fails loud. - M2: stack invariant checks. Every member of an MRS series must agree with d0 on isMRS, dataPointColumns, spectralWidth, isLittleEndian, manufacturer, isXA. Mismatch aborts before any FID is read. - M3: affine validity gate. Zero / NaN / Inf in orient or position, or non-positive voxel spacing, now sets sform_code=0 with a printWarning instead of stamping an authoritative-but-bogus geometry. - L2: foreign save formats rejected at MRS dispatch with a direct diagnostic, so MGH/NRRD/BJNIfTI don't read the FID just to fail at the writer (and bypasses the BJNIfTI complex-split bug noted in L3). - L1: ResonantNucleus emission now uses json_Str instead of raw fprintf so a malformed DICOM CS containing a quote or backslash gets escaped rather than breaking JSON. tools/reproinx.py: - H1: _purge_all_discard_unknown() refuses to rmtree when any Unknown/ child is a directory. dcm2niix doesn't write nested dirs there, but an external tool / crashed run could; the previous code would have deleted them unread. - M1: "mrs" added to _BIDS_DATATYPES with an explicit comment tying it to the C-side BidsGuess emission. Any MRS series that falls into Unknown/ via a future failure path can now be rescued. - M4: _bidsguess_demote_3d_bold preflight now checks BOTH .nii and .nii.gz at the target stem so a `_sbref.nii` blocks a `_bold.nii.gz` demote (and vice versa). Docstring corrected — bold files don't carry .bvec/.bval, so the rename loop doesn't iterate them. CLAUDE.md: - New "Audit-deferred items from 2026-06-05 follow-up review" section persisting the M5 (saveDcm2NiiMRS handwritten mini-pipeline), M6 (no in-repo coverage for VIBE/SWI/purge), L3 (BJNIfTI complex split inconsistency), and L4 (Siemens classifier cascade complexity) rationales. Verified: - cmake build clean. - In-tree regression (dcm_qa, dcm_qa_nih, dcm_qa_uih) clean — zero new BidsGuess deltas, zero unexpected sidecar diffs. - dcm_qa_mrs PASS. - MRS Phase B byte-identity still passes against /Users/chris/src/spec2nii/XA60/Ref/series30.nii.gz (524288 bytes identical) — confirms the new endian/geometry/dim validation didn't drift the writer. audit_response.md written locally (gitignored per project convention). audit_temp.md removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Third 2026-06-05 audit cycle (no external audit_temp.md this round; three
internal agents covered security/bugs, refactor, and docs). One small
code fix landed, several refactor opportunities documented but not
acted on per "do no harm".
Code (nii_dicom_batch.cpp):
- M1: corrected the misleading "<=4 chars" comment in setBidsHeuristics'
task-hint block. The wordBoundary flag is used by drift-prone tokens
including "motor" (5 chars), not just <=4 chars. The comment now
explains the actual criterion ("letters do not appear inside common
English words") so future readers don't try to "fix" the boundary
flag on motor.
CLAUDE.md (docs agent updates):
- Unknown/-rescue subsection notes the 2026-06-05 audit H1 hardening
(iterdir loop now refuses rmtree when any child is a directory).
- _BIDS_DATATYPES description tied to the audit M1 fix that added
"mrs" to the allowlist, with a note that BEP005 is not in the
stable BIDS spec as of 2026.
- MRS pipeline subsection acknowledges dcm_qa_mrs as out-of-tree
coverage and lists the seven Phase-B audit hardenings
(H2 endian, H3 DT_COMPLEX64 swap, H4 dim overflow, M2 stack
invariants, M3 affine validity, L1 ResonantNucleus json_Str,
L2 foreign save-format rejection).
Deferred refactor opportunities (logged in audit_response.md, not
implemented):
- Extract mrs_validate_stack and mrs_build_affine helpers from
saveDcm2NiiMRS (low effort, deferred until Phase C lands and the
extraction is required for MRSI/Unloc/mrsref).
- bidsExtractTokenAfter helper to absorb the duplicated task-/acq-
word-boundary walk loops.
- Extract setBidsSiemens_b1map as the starter refactor for the L4
Siemens classifier cascade simplification.
- Consolidate constants section in tools/reproinx.py.
- Consolidate the two CLAUDE.md audit sections into one chronological
log.
These are quality improvements, not bug fixes. Per "do no harm" the
recent codebase change cadence already cleared two audit cycles of
real findings — speculative refactoring now would churn diff context
without payoff. Revisit when Phase C MRSI/Unloc/mrsref lands and the
saveDcm2NiiMRS extraction becomes a forcing function.
Verified:
- cmake build clean.
- In-tree regression (dcm_qa, dcm_qa_nih, dcm_qa_uih) clean — zero
new BidsGuess deltas, zero unexpected sidecar diffs.
- dcm_qa_mrs PASS.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ts + AcquisitionVoxelSize + NumberOfTransients dcm2niix's MRS JSON sidecar now carries the four BIDS-MRS REQUIRED fields (ResonantNucleus, SpectrometerFrequency, SpectralWidth, EchoTime) plus three of the most useful RECOMMENDED fields, so the output is BIDS-MRS compliant without needing the NIfTI-MRS header extension. Reference: https://bids-specification.readthedocs.io/en/stable/modality-specific-files/magnetic-resonance-spectroscopy.html In setBidsSiemens's MRS-emit block (nii_dicom_batch.cpp:~2981): - TransmitterFrequency renamed to SpectrometerFrequency. Same value (DICOM 0018,9098 FD parsed into d.imagingFrequency), but BIDS-MRS uses the latter as the canonical key. The BEP005 NIfTI-MRS extension also uses SpectrometerFrequency. - SpectroscopyAcquisitionDataColumns renamed to NumberOfSpectralPoints. Same value, BIDS-MRS canonical key. - NEW AcquisitionVoxelSize: [x, y, z] in mm, sourced from xyzMM[1..3] which the parser projects from PixelSpacing[0/1] + SliceThickness. - NEW NumberOfTransients: count of averages stacked along NIfTI dim[5]. Read from the header passed to nii_SaveBIDSX so it agrees with what saveDcm2NiiMRS actually wrote. The previous keys (TransmitterFrequency, SpectroscopyAcquisitionData- Columns) are removed rather than dual-emitted because they were only in two commits and no external tool depends on them; carrying both would clutter the sidecar. Why this is enough for BIDS-MRS, no NIfTI extension yet: The BIDS-MRS spec says the JSON sidecar is the authoritative location for the four REQUIRED fields. The NIfTI-MRS header extension (BEP005, ecode 44) is expected by tools that don't parse sidecars (raw FSL-MRS, spec2nii's spec2graph). The companion change to dcm_qa_spec/spec2graph.py teaches it to read the sidecar first and fall back to the extension, so dcm2niix output renders correctly without an extension writer. A future commit can add the extension for tools that haven't been updated. Verified: - cmake build clean. - In-tree regression (dcm_qa, dcm_qa_nih, dcm_qa_uih) clean. - /Users/chris/src/dcm_qa_spec/spec2graph.py with our output now produces a chemical-shift (ppm) axis identical to the spec2nii reference render at the 2.0-3.3 ppm metabolite region (Choline / Creatine peaks at the correct locations). - dcm_qa_mrs Ref/ JSON would need refresh; the .nii files are still byte-identical because the changes are sidecar-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
bids-validator hardcodes "n/a" as the only accepted missing-value marker in TSV cells and explicitly rejects "nan", "NaN", "NA", "na" with TSV_VALUE_INCORRECT_TYPE plus a "did you mean 'n/a'?" hint (see bids-core/src/tables.rs:679-682 nan_hint_regex and :685-688 check_value). dcm2niix's physio writer was emitting the literal "nan" string for sample dropouts — a deliberate choice for byte-parity with bidsphysio / pandas but now an explicit validator error on the output. Two TSV emission sites in xaPhysioWriteStreamFiles updated (trigger + no-trigger branches). bidsphysio parity is retired in favor of validator compliance; downstream code that previously parsed our output as float32 with NaN-fill should now treat the "n/a" token specifically (pandas read_csv with na_values=["n/a"]). Regression: dcm_qa/dcm_qa_nih/dcm_qa_uih show only timing variance vs prior baseline. No MRS impact (separate code path). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous one-pass collision handler in _rescue_unknown_dir gave
the first file at a given target stem the un-numbered name and later
siblings _run-01, _run-02, ... Two problems:
1. Order was determined by sorted(unknown.glob("*.json")) — alphabetic
by filename. dcm2niix's Unknown/ filenames embed the SeriesNumber as
a prefix, so this matched scanner acquisition order ONLY by string-
sort coincidence: series 30 vs 31 ("30..." < "31...") works, but
series 2 vs 10 ("10..." < "2...") and series 5 vs 32 ("32..." < "5...")
inverted. The "earlier" file by acquisition would silently get the
later run number.
2. Asymmetric output: one sibling at `_svs`, the other at `_run-01_svs`.
Downstream BIDS tooling has to special-case the implicit-run-zero
convention.
Replace with a two-pass design:
Pass 1 — for every json in Unknown/, compute (target_dir, base_stem,
run_template, src_stem_name, SeriesNumber). All existing
validation (datatype allowlist, entity charset, task
recovery for func/_bold, provenance lookup) preserved.
Pass 2 — group candidates by (target_dir, base_stem). Single-member
groups land at the un-numbered base_stem (current behavior
when there's no collision). Multi-member groups land at
_run-NN for every member, with NN assigned by ascending
SeriesNumber — scanner acquisition order, deterministic
and reproducible across runs.
An existing on-disk family at base_stem (re-run, or C-side direct
write) counts as a collision: new rescues get _run-NN from the lowest
free index, but the pre-existing file is NOT touched. Avoids
surprises on re-runs at the cost of perfect symmetry in that edge
case.
Verified against puzzle/svs/20260605/* (two SVS, series 30 + 31):
before: _svs (series 30) + _run-01_svs (series 31)
after: _run-01_svs (series 30) + _run-02_svs (series 31)
BIDS validator clean both before and after (0 errors); the change
fixes naming determinism, not validation.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
External review at HEAD 2499188 surfaced 4 HIGH + 10 MEDIUM + 8 LOW items (audit_temp.md → audit_response.md). This commit lands the real-bug subset plus the cheap LOW fixes; everything else is deferred with rationale in audit_response.md. H1 (FIXED) — classic Siemens MRSI sidecar contradicted the suffix. Files routed through saveDcm2NiiMRSI with mrsAcqType==kMRSAcqNone (classic VB/VE CSI on the CSA Non-Image SOP) got BidsGuess ["mrs","_mrsi"] but the sidecar fell through to ScanningSequence "Unlocalized MRS" with no MRSpectroscopyAcquisitionType emitted — filename and metadata disagreed. Derive mrsAcqType from xyzDim before nii_SaveBIDSX so the existing emission picks the right MRSI branch. Verified on sm_classic: now emits MRSpectroscopyAcquisitionType: VOLUME + ScanningSequence: MRSI. H3 (FIXED) — MRSI per-axis dim limit gate. The size_t cols*rows*slices*N_pts*8 multiplication could wrap on a pathological input and pass the post-allocation byte-count check silently. NIfTI-1 int16 dim limit guard added before allocation (mirrors SVS pattern at ~11580). H4 (FIXED) — MRSI big-endian payload byte swap. SVS swaps at ~11922 via nifti_swap_4bytes; MRSI did not. Mirrors the SVS idiom post-fread, gated on !d0->isLittleEndian. Rare in practice (legal per DICOM, no corpus driver). M3 (FIXED) — CSA string helpers null-deref on malloc failure. Three sites: csaMultiFloat (~1392), csaMultiDouble (~1423), csaICEdims (~1459). All used malloc → memcpy with no null check. Fix: skip-on-OOM without touching the output buffer or advancing ItemsOK (csaICEdims returns -1 to match its existing missing-coil-number sentinel). L1 (FIXED) — BidsGuess emission checked bidsDataType twice (typo). Second check is bidsEntitySuffix, so a future path that sets the datatype but leaves the suffix empty no longer emits ["mrs",""]. L2 (FIXED) — stale spec_plan.md references after the round-5 deletion. Three source comments + one CLAUDE.md line redirected to dcm_qa_mrs/caveats.md (which carries the institutional memory now). DEFERRED (see audit_response.md for full rationale): - H2 UIH non-square MRSI axes mismatch (corpus-clean). - M1 weak geomValid + M2 VOI hasVoiCenter sentinel (corpus-clean). - M4-M6 mrs_post.py UX/robustness (Python-side; separate cycle). - M7 NumberOfTransients semantic for MEGA (spec discussion). - M8 ReproIn rename transaction + M9 physio JSON-before-TSV order (real but rare; deferred). - M10 MRSI 2x memory (design trade-off, acceptable). - L3-L5 cosmetic docs. - L7 reproinx unit tests + L8 pinned scoreboard SHAs (workflow). Regression: dcm_qa/dcm_qa_nih/dcm_qa_uih show only standing baseline diffs. MRS scoreboard unchanged at 23/30 PASS bare, 30/30 PASS via --with-mrs-post (H1 fix shifts sm_classic's JSON from "missing field" to "consistent with suffix"; was already passing parity-wise). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The literal `.duecredit.p` line was copied verbatim from heudiconv's reproin scaffolding (heudiconv/bids.py:205) as part of the byte-identity goal. heudiconv writes it because the duecredit Python package leaves a `.duecredit.p` pickle in the cwd when DUECREDIT_ENABLE=yes. Neither dcm2niix (C++) nor reproinx.py imports duecredit, so the file never exists on this code path and the .bidsignore rule matches nothing. Drop the unconditional write. The collision-suffix / single-volume-DWI / bidsguess-residuals passes still create .bidsignore on demand when there is something real to ignore. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CMRR-format Siemens PhysioLog encodes PULS_TRIGGER / RESP_TRIGGER / EXT_TRIGGER / ECG_TRIGGER events as 4-column rows with VALUE=2048 inserted at off-grid odd ticks. The previous parser dropped the 4th SIGNAL token but still pushed 2048 into the cardiac / respiratory sample stream — visible as sharp spikes that distort HR estimation and spectral analysis. Same bug remains in bidsphysio (dcm2bidsphysio.py:213 "we can ignore"); Siemens reference physiodcm2tsv.py separates them by row width. Follow the Siemens-reference policy: a 4-column row with a `_TRIGGER` SIGNAL is captured into a per-stream `triggerTics` array (not pushed as a sample). At emit time the per-stream triggers are merged with the global ACQUISITION_INFO volume tics and rasterised onto the trigger column via the same nearest-grid snap. Cardiac peaks land in cardiac_physio.tsv.gz, respiratory peaks in respiratory_physio.tsv.gz, volume triggers in both — all as `1` (no ambiguity since the two streams live in separate TSVs). Validation: dcm_qa_physio In/7 cardiac now shows 31 trigger rows (27 PULS peaks + 4 volume) instead of 4, with zero 2048 sentinels in the waveform column; respiratory shows 13 (9 RESP + 4 volume). The companion regression set (neurolabusc/dcm_qa_physio) documents the encoding and links to the bidsphysio bug for parser authors hitting the same case. Also: drop empty console/pypeline.log (accidental zero-byte file from the 2023 BidsGuess commit, no references anywhere); update two stale "Phase 6 work" markers in CLAUDE.md since the MRSI writer is live. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous fix merged ACQUISITION_INFO volume triggers and per-row PULS_TRIGGER / RESP_TRIGGER markers into a single trigger column, emitting `1` for either. That's lossy and quietly deviates from the BIDS spec's "continuous measurement of the scanner trigger signal" description of the `trigger` column. bidsphysio's column 2 is unambiguously the scanner pulse train (physio peaks discarded); merging the two would make column 2 mean something different per converter. Emit three columns instead. cardiac_physio.tsv.gz columns become [cardiac, trigger, cardiac_trigger]; respiratory_physio.tsv.gz becomes [respiratory, trigger, respiratory_trigger]. The BIDS-canonical `trigger` column reverts to scanner-only `0/1` (byte-compatible with bidsphysio); the new `<label>_trigger` column carries the firmware-detected R-wave / respiratory events the previous fix already captured per-stream. The JSON sidecar's Columns array documents the layout; readers that look up entries by name pick up the new column, readers that hard-code n_columns==2 keep working since column 2 is unchanged. Implementation: physioBidsFillUniform now takes a second tic array and allocates a second uint8_t rasterised output. xaPhysioWriteStreamFiles accepts a `peakLabel` + `peakTrigger` pair and writes a 3-column TSV when non-NULL. physioBidsEmitStream forwards the per-stream peakTics. The XA-line caller passes NULL since the gzip-XML PhysioLogging format has no per-row trigger markers (its <PhysioTriggers> element is a separate parser path not wired in yet). Validation: dcm_qa_physio In/7 cardiac column 2 now has 4 rows (scanner volumes only); column 3 has 27 (PULS_TRIGGER). Respiratory column 2 has 4; column 3 has 9 (RESP_TRIGGER). No information lost, no merge ambiguity, spec-aligned trigger semantics. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… docs External review (audit_temp.md) and three in-house agents found one real bug from the column-split commit, one naming wart, and several stale comments. This commit closes them out. Bug fix (H1): xaPhysioWriteStreamFiles previously gated the 3-column TSV emission inside the `trigger != NULL` branch. A stream with a non-NULL peak channel but no ACQUISITION_INFO declared `[label, peak_label]` in the JSON Columns array while writing a single-column TSV. Silent data loss. Refactored the TSV body to build rows column-by-column (one independent snprintf-append per non-NULL pointer), so the four schemas — signal / signal+trigger / signal+peak / signal+trigger+peak — fall out of one code path. Doubles as the refactor agent's #1 readability win. Defensive (Security-agent H2): physioBidsRasterTrigger silently returns NULL on calloc OOM, which previously degraded the peak channel to "no column" without any user notice. Added a printWarning when the column was promised (peakN>0) but the rasteriser couldn't allocate. Naming wart (L2): EXT streams' BIDS label is already "external_trigger" (the entire channel is a TTL pulse train), so the previous "<label>_trigger" pattern produced "external_trigger_trigger". Use "_peak" suffix for EXT, "_trigger" for PULS/RESP/ECG. Stale-comment cleanup (L1): three sites still said per-stream triggers were "merged with the global ACQUISITION_INFO volTics at emit time" from the pre-split design. Updated to reflect the dedicated <label>_trigger column. Docs sweep: - CLAUDE.md: new "Siemens PhysioLogging / CMRR PMU TSV" bullet under "Sidecar emission gotchas" covering the 3-column layout, the VALUE=2048 off-grid sentinel encoding, file:line citations, the XA-line 2-column fallback, and the dcm_qa_physio In/7 expected cardinality. - README.md: existing physio bullet expanded to mention the 3-column TSV layout, byte-compat with bidsphysio on column 2, link to dcm_qa_physio for full details. Regression: dcm_qa_physio Ref/ output byte-for-byte unchanged (4+27 cardiac scanner+peak triggers, 4+9 respiratory). Full review disposition tracked in audit_response.md; carryforward list at the bottom of that file covers H2/H3, M1+M2+M3, M5–M11, L3+L5, L6 (all pre-existing or future-cycle work). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Second physio audit cycle. The external review and three in-house agents found: - One real fix: physioBidsFillUniform was allocating uT (scanner trigger raster) even when volN == 0. The writer already gated column 2 on volN > 0, so the allocation was unused — and could silently drop a signal+peak stream on calloc OOM. Now gated. - Several docs drifts after the cycle-1 EXT _peak rename + .bidsignore removal. README, CLAUDE.md, REPROIN.md, tools/reproinx.py docstring, tools/mrs_post.py dep note all swept. - CLAUDE.md physio bullet was too verbose (220 words, file:line citations that rot fast). Trimmed to 105 words while keeping load-bearing knowledge: 3-column layout, VALUE=2048 sentinel, bidsphysio anti-pattern reference, dcm_qa_physio pointer. - README MRSI claims softened to mention single-DICOM-only Enhanced CSI / UIH MRSI and the non-square UIH gap. No regressions: dcm_qa_physio Ref/ output byte-for-byte identical (4 + 27 cardiac scanner+peak, 4 + 9 respiratory). The Security agent confirmed the cycle-1 flatten is correct (no buffer underflow, no partial-row leak, XA-line path preserved). The Refactor agent flagged the CLAUDE.md trim (applied by Docs). The Docs agent applied all docs fixes plus the EXT _peak exception mention in README + CLAUDE. Pivot: two consecutive cycles have exercised the physio path thoroughly. The carryforward (audit_response.md) lists MRS hardening as the next focus — UIH non-square MRSI dim/payload mismatch is the highest priority remaining item. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
H2 UIH non-square MRSI dim/payload mismatch — swap dim[1]/dim[2] in the
UIH branch of saveDcm2NiiMRSI so the header matches the r-innermost
transpose. Square corpus byte-identical with spec2nii; non-square
synthetic (rows=8, cols=16) now matches spec2nii too.
M8 VOI sidecar presence sentinel — add hasVoiCenter bool to TDICOMdata,
populate at the CSA + Enhanced MidSlabPosition paths, gate emission so
partial-CSA Enhanced SVS (SlabThickness set, no MidSlabPosition) no
longer emits a fabricated [0,0,0] VOI translation.
M9 MRSI dim_5 gate — suppress "dim_5: DIM_DYN" when mrsAcqType is
ROW/PLANE/VOLUME so the BIDS-MRS sidecar matches saveDcm2NiiMRSI's
dim[0]=4 header. SVS still emits (spec2nii Siemens does the same).
M1+M2 physio writer error propagation — xaPhysioWriteStreamFiles now
returns bool with cJSON / fopen / fwrite-short / deflate checks and
unlinks partial files. physioBidsEmitStream returns a tri-state
PhysioEmitStatus distinguishing skip ("no sensors") from failure
(rasterizer OOM, write error). Callers tally writeFails and propagate
EXIT_FAILURE.
H1 reproinx physio sweep gate — _rescue_physio_recordings returns
(rescued, skipped); _drop_derivatives takes a skip_roots arg and
preserves derivatives/scanner/ on roots that still have un-rescued
physio (multi-session, collision, OSError). Loud warn per root with
the --keep-derivatives escape hatch.
Validation: dcm_qa_mrs 23 pass / 7 fail (sLASER + MEGA-PRESS only,
unchanged); dcm_qa / dcm_qa_nih / dcm_qa_uih NIfTI images unchanged;
dcm_qa_physio decompressed TSV unchanged; synthetic H1 harness
exercises both collision-skip and clean-rescue scenarios.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Extend the M1/M2 physio writer fail-closed work to the remaining
OOM/overflow/malformed paths so memory pressure or hostile input can no
longer produce silently-truncated output reported as EXIT_SUCCESS:
- TSV row overflow: declare the row index outside the loop and fail
closed (unlink + return false) when not all nSamples rows serialized,
instead of gzip-writing a short TSV with success status.
- Signal-buffer and scanner-trigger-raster OOM in physioBidsFillUniform
now set a bool *outOom out-param; the caller maps it to
kPhysioEmitFailed rather than collapsing OOM into the degenerate-input
Skipped path (which returned EXIT_SUCCESS).
- CMRR ACQUISITION_INFO ACQ_START tic and XA XML ACQUISITION_TIME_TICS
volume tic now use strtol+endp (consumed-the-start check only, so
trailing-whitespace XML attrs stay tolerated) instead of atol, which
would collapse a malformed token to 0 and poison volTics[0] (the
StartTime anchor) / fabricate a tic-0 scanner trigger.
- cJSON sidecar: verify the void-returning AddItemTo{Array,Object} calls
actually landed (column count + the three required keys) before
trusting the JSON; a key-strdup OOM would otherwise drop a
BIDS-required field while cJSON_Print serialized success. Uses the
public cJSON API only; the bundled lib is unmodified.
Deliberately declined (security agent): full-token *endp=='\0' numeric
checks (regress XML trailing-whitespace tolerance), path-construction
truncation guards (PATH_MAX + short fixed labels, unreachable), and the
tsvLen > UINT_MAX gzip guard (needs a 4 GB stream).
Docs: fix CLAUDE.md dim_5 SVS-vs-MRSI contradiction, README VOI-matrix
overstatement, and the stale physioBidsEmitStream return-semantics
comment; record the cycle-3/4 fixes + declined-items rationale in the
CLAUDE.md physio error-propagation bullet.
dcm_qa_physio output byte-identical on valid data (fixes only alter the
OOM/overflow/malformed paths).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Physio BIDS sidecar completeness:
- Emit PhysioType: "generic" (BIDS RECOMMENDED) and verify it landed in the
cJSON post-attach key check alongside Columns/SamplingFrequency/StartTime.
- Document the non-standard firmware-peak column (e.g. cardiac_trigger) with a
best-effort Description object so the validator stops warning
TSV_ADDITIONAL_COLUMNS_UNDEFINED.
- reproinx.py: backfill physio TaskName from the filename task- entity
(_ensure_physio_tasknames, Pass 3) — the dataset-level task-X_bold.json
TaskName is not inherited across the _physio suffix.
Physio fail-closed finite/bounds guards (audit cycles 5–6):
- physioBidsFillUniform bounds the raster length in the double domain before
the int cast (reject non-finite expD and expD outside [1, INT_MAX-2]) so a
hostile tic span cannot overflow the cast or under-allocate.
- Reject non-finite dtMs at the entry guard: a malformed CMRR SampleTime
("inf"/"nan" via atof) would otherwise pass >0/<=0 tests, make dtTics
non-finite, collapse expD to 1.0, and map every sample to index ~0 — a
corrupt-but-"successful" TSV. XA dtMs is integer-tic-derived (immune).
- TSV writer renders any non-finite signal value as BIDS n/a (!isfinite, not
just isnan), so a hostile inf sample token can't print a literal "inf".
ASL:
- 3D tgse_pcasl: emit a single LabelingDuration from the explicit adFree[2]
value; drop the duplicate NumRFBlocks*18.4ms emission (that formula is the
2D ep2d_pcasl heuristic). Fixes an invalid duplicate JSON key.
- Cite the 18.4 ms/RF-block constant (Korean J Radiol 2018, doi:10.3348/
kjr.2018.0651) at the 2D pCASL site.
Docs: CLAUDE.md physio fail-closed contract restructured into durable
invariants + the finite-timing guard.
dcm_qa_physio output byte-identical on valid data (guards fire only on
malformed/hostile input); codespell + dcm_qa clean (only pre-existing
version drift).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BidsGuess / -f %h naming (BEP009 PET stable, BEP-024 CT datatype/suffix casing): - PET: emit lowercase BidsGuess ["pet","_pet"] instead of ["PET","PET"]. The old uppercase produced an invalid "PET/" dir and a separator-less "<sub>_<ses>PET" filename the BIDS validator rejects (NOT_INCLUDED), and the recognized-as-PET file also cleared a spurious NIFTI_PIXDIM warning (the rule exempts suffix==pet, so pixdim4=0 dynamic PET is fine). - CT: emit lowercase ["ct","_ct"] (datatype/suffix casing is the stable part of BEP-024). reproinx's _BIDS_DATATYPES still omits "ct" so -f %H keeps CT in Unknown/ (the validator has no ct datatype yet) — documented as a deliberate exception so a future "alignment" cleanup doesn't break %H. ScatterFraction: emit as a single-element array [%g]. The BIDS-PET schema types it strictly as array (unlike ReconFilterSize which is number-or-array), so a scalar is a validator type error; matches PET2BIDS and the sibling DecayCorrectionFactor/FrameDuration arrays. PET TimeZero / InjectionStart: explicitly NOT emitted (preserve issue #983 / PR #1014). dcm2niix emits raw SeriesTime and lets a PET-BIDS finalizer (PET2BIDS) choose the time-zero convention; the old TimeZero used AcquisitionTime (wrong for dynamic/multi-bed/time-subset reconstructions) and Siemens RadiopharmaceuticalStartTime is dose-measurement time (not injection). Replaced the commented-out blocks with a guard comment + CLAUDE.md note so this isn't re-enabled by a future audit; stale parser/comment references to InjectionStart corrected. ADMIN ImageDecayCorrectionTime keeps using RadiopharmaceuticalStartTime (the DICOM-defined decay reference — a different, defensible semantic). Version: v1.0.20260617. dcm_qa + codespell pre-push gate clean (only pre-existing stale-Ref drift, unrelated to these PET/CT changes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validated `tools/reproinx.py` against BIDS-Manager (Oldenburg samples), bidscoin, and a physio corpus; hardened over three audit cycles. reproinx.py - Unknown-rescue recovers reproin entities the one-pass C parser can't name (session-first / suffix-last protocols that heudiconv itself rejects): study-scoped session from protocol, numeric-run honouring with BIDS-ordered insertion, and `_strip_orphan_runs` for non-disambiguating runs. - Cross-series reclassification preserves anatomical dir-AP/PA (was clobbering with voxel-axis dir-J/Jn); half-session consolidation restricted to BIDS datatype dirs. - Unknown- and derivatives-physio recordings paired to their BOLD and moved as atomic families; subject resolved fail-closed from the sanitized provenance OutputStem (no cross-subject mis-pair); session excluded from the match key so sessionless protocols still pair. - `_move_stem_files` is now the shared all-or-nothing family-move primitive (preflight + best-effort rollback), adopted by the imaging rescue, both physio rescues, orphan-run strip, and reclassification. C side (no functional change to the default build) - PET ADMIN ImageDecayCorrectionTime comment: DICOM administration-time / decay-reference semantic, not an injection-time assertion (issue #983). - remove_specialchars(): new[] now paired with delete[] (wrapper-only path). - version bump v1.0.20260620. Docs: CLAUDE.md / REPROIN.md / README.md updated to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ession from 54fe303) Commit 54fe303 ("initTDTI4D() helper closes nii_SaveBIDS latent UB") replaced the narrow per-stack reset in saveDcm2NiiCore with the broad initTDTI4D() helper. At that site dti4D has ALREADY been populated by the enhanced/ multiframe parser (and PAR/REC), so initTDTI4D's extra writes clobbered three LIVE per-frame fields: - sliceOrder[0] -> gates enhanced per-slice reorder (nii_dicom.cpp ~4430) - intenScale[0] -> gates enhanced per-slice rescale (nii_dicom.cpp ~3663) - triggerDelayTime[0] -> ASL post-label delays The result was scrambled pixel data for ALL Philips/Canon enhanced 4D output (DTI / BOLD / ASL / fieldmap): e.g. a resting-state BOLD whose volume means should be flat ramped 121k->188k, and Canon enhanced DTI lost its b0-high structure. Caught by bisecting the dcm_validate corpus against the v1.0.20260416 release (byte-identical to the stale Ref) — first bad = 54fe303. Fix: restore the narrow reset at this one site (only the 4 BEP009/PET sentinel arrays + the two repetition-time scalars). The other three initTDTI4D call sites build a TDTI4D from scratch and are correct. Verified: - dcm_validate enhanced modules (canon_enh, canon_61, philips, philips_dwi, philips_enh, philips_asl_enh, enh) now pixel-identical to Ref - issue #1015 high-slice CT (1252-slice FFS) unchanged: srow_z=(0,0,0.5,614.5) correct; that fix lives in sliceTimingCore (1cd1620) and is independent - dcm_qa / dcm_qa_nih / dcm_qa_uih: no image/file diffs (sidecar-only drift) - codespell clean CLAUDE.md: correct the initTDTI4D() contract to forbid the helper at the saveDcm2NiiCore site and document the live-field hazard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dcm2niix now embeds the NIfTI-MRS hdr_ext JSON (ecode 44) inside MRS .nii(.gz) in addition to the BIDS sidecar, so a tool can read SVS/MRSI metadata directly from the NIfTI — useful for drag-and-drop into sandboxed environments where the paired .json is unreadable. Mirrors spec2nii (https://wtclarke.github.io/mrs_nifti_standard/). Validated to 30/30 ecode-44 key-parity vs spec2nii across the DICOM-MRS corpus (Siemens VB/VE/XA SVS + sLASER + CSI/MRSI, Philips classic+enhanced SVS, UIH SVS+CSI), measured by the new dcm_qa_mrs/compare_ext.py harness; nifti_mrs validator loads SVS + MRSI. Writer: - nii_saveNII() gains an optional `const char *hdrExt = NULL`. When set it writes extender + esize + ecode(44) + null-padded JSON between the 348-byte header and the image, sets vox_offset and intent_name="mrs_v0_11". hdrExt==NULL writes the historic 4-byte terminator, so non-MRS output is byte-identical (verified: 0 binary diffs across dcm_qa/nih/uih). All format branches (zstd/internal-gz/pigz/ raw) carry it via one shared extBlock; esize/ecode byte-swap under --big-endian. - writeNiiGz() now returns int and never frees its src_buffer (caller owns it), fixing a prior leak + latent double-free. - mrsHdrExtJson() builds the JSON from already-parsed TDICOMdata: required SpectrometerFrequency/ResonantNucleus, dim_5, TxCoil/RxCoil, SequenceName, EchoTime/RepetitionTime/InversionTime, VOI (via shared mrsVoiMatrix() helper), WaterSuppressed, provenance. PII (patient block + OriginalFile) gated to match the sidecar (-ba y/-ba o). Returns NULL (→ plain NIfTI) when required fields are absent or on cJSON OOM. - readCSAforMRS() extracts the CSA SequenceName element for VB/VE/sLASER/CSI classic MRS (public 0018,0024 absent), matching spec2nii and filling the BIDS sidecar too. MRS-gated; non-MRS Siemens output unchanged. Hardening (4 audit rounds): write-status contract — every save path fails closed on short fwrite/fclose/pclose/compressor errors (incl. Windows pigz exit code) and removes the truncated file, so no sidecar is emitted next to a missing/corrupt image; esize computed in size_t with an INT_MAX guard; cJSON fail-closed verification; zstd big-endian error-path buffer restore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- kDCMdate -> v1.0.20260622 so release artifacts (and the MRS extension/sidecar provenance) report the correct date. - writeNiiGz(): add a single ferror() check covering the gzip header-magic/trailer fputc + payload fwrite, mechanically completing the write-status contract (the payload short-write + fclose checks were already present; a deflate failure is caught by the cmp_len<=0 guard). - pigz pipe: remove the partial <name>.nii.gz on a write/pclose failure, matching the raw/zstd/internal-gz cleanup. - README.md: add nipoppy to the downstream-tools list (clears the dirty tree for a clean release artifact). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- dcm_qa_nih d302d6e -> 6a11dc6, dcm_qa_uih d729bdd -> f3bd991. - Both now ignore Out/ (the regression output batch.sh writes), matching dcm_qa, so a regression run no longer leaves the superproject tree dirty. - The bump also picks up the Ref refresh for the current dcm2niix; in-tree regression now passes clean (0 image diffs, 0 sidecar diffs) for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audit follow-up: hold the legacy MGH/MGZ and NRRD output paths to the same write-status/ownership contract as the main NIfTI path, and fix a long-standing corruption bug the audit surfaced. - writeMghGz(): every .mgz was CORRUPT. The deflate stream was finalised with Z_FINISH on the IMAGE and then the footer was deflated with Z_NO_FLUSH (a no-op after finalize), so the footer was omitted from the compressed stream while the gzip CRC/ISIZE still counted it -> the decompressor always reported a CRC error (reproduced on the prior commit). Z_FINISH now applies to the footer (last chunk). Verified: .mgz is valid gzip, decompresses byte-identical to .mgh, and nibabel loads it. - writeMghGz() hardened to match writeNiiGz(): returns int, NULL-checks pCmp, guards compressBound > UINT_MAX, never frees caller-owned src_buffer, checks the payload short write + ferror + fclose, and removes a truncated .mgz. - nii_saveMGH(): propagates writeMghGz status, checks the uncompressed .mgh fwrite/fclose, restores im endianness on the error path, removes partial output. - nii_saveNRRD(): checks the header fprintf (ferror) + fclose, the uncompressed image write, and the external-pigz .raw staging fopen/fwrite/fclose; all fail closed and remove partial files. - writeNiiGz(): also guard compressBound > UINT_MAX (the ~4 GiB internal-gzip gap below kMaxGz where avail_out would wrap). Verified: MRS 30/30 parity, non-MRS dcm_qa/nih/uih 0 image diffs, codespell clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…status Audit follow-up closing the two named advertised-format release blockers plus related completeness items. - nii_saveNRRD(): the header fopen() was unchecked — a permission error / bad path / fd exhaustion crashed via fprintf(NULL) on the `-e y` path. Now NULL-checked, fail closed. Verified: read-only output dir exits cleanly (no crash). - writeMghGz(): the compress bound was compressBound(src_len + sizeof(hdr)) but the stream also includes sizeof(footer); now bounds header+image+footer. The three deflate() calls are chained on Z_OK and a final Z_STREAM_END is required, so a Z_BUF_ERROR from an undersized bound fails closed instead of emitting a gzip wrapper whose CRC/ISIZE over-counts. - writeNiiGz(): likewise require Z_STREAM_END after Z_FINISH (atop the existing compressBound/UINT_MAX guard). - nii_saveNRRD(): on a pigz data-step failure, remove the orphaned .nhdr header so it never points at a missing/partial .raw.gz. Verified: MGZ valid + round-trips, NRRD valid, NRRD crash path clean, MRS 30/30 parity, non-MRS dcm_qa/nih/uih 0 image diffs, codespell clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Final audit cleanup (reviewer signed off; these are caveat/cleanup items after already-failing operations, no data-corruption or false-success). - nii_saveNRRD(): on a gz data-step failure (internal-gz, no-zlib, or external pigz), remove the detached .nhdr header so it never points at missing/partial .raw.gz data. This also fixes a latent bug from the prior cleanup: the header name was captured from `fname` AFTER the header writer had reassigned it to <stem>.raw.gz, so the wrong file was removed on failure; the header name is now derived from niiFilename. - nii_saveNRRD(): the unsupported-datatype path now removes the partial header it already wrote. Verified: NRRD .nhdr+.raw.gz valid, MGZ valid, MRS 30/30 parity, non-MRS dcm_qa/nih/uih 0 image diffs, codespell clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AppVeyor retired the Ubuntu1604 worker image and substituted a modern one that no longer ships gcc-8, so the pinned `export CC=gcc-8 CXX=g++-8` made cmake fail with "Could not find compiler set in environment variable CC". Move the Linux job to the current Ubuntu2004 image and drop the dead gcc-8 pin (use the image default gcc-9). Windows/macOS jobs unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#1024 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…DS writer; WASM worker exit fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Multi-echo fMRI with phase collides 8 series onto one %H stem (dcm2niix appends a/b letters and can misname a phase SBRef _bold). Add a cross- series pass that renames the family to _part-<mag|phase>_<bold|sbref>: - part from BidsGuess's explicit _part- (lookahead regex so a shared '_' between two component tokens is not consumed, keeping ambiguous multi- component guesses ambiguous), falling back to ImageType; suffix from BidsGuess so a phase SBRef in a _bold name resolves correctly. - Group renames are all-or-nothing with rollback. A bold/sbref file whose part or suffix cannot be inferred taints its whole prefix, so no firing group at that prefix is partially resolved (leaves collision names for .bidsignore rather than an inconsistent _bold + _part-phase_bold pair). - Siemens phase sidecars get Units: "arbitrary" (scanner-scaled, not radians); never overwrites an existing Units, idempotent across reruns. - _emit_events_tsv strips _echo- and _part- so one run-level _events.tsv is shared across echoes/parts. - _apply_dup_naming validates every group member (including the unsuffixed base) before the two-phase rename, so a missing base + surviving sibling no longer produces a base-less __dup family. Adds regression coverage (part precedence/idempotence, ambiguous skip, unclassifiable-sibling family skip, events one-per-run, dup missing-base) and documents the pass in docs/BIDS_REPROIN.md. Verified: reproinx self-check green; ~/src/bidsx and dcm_qa_3depi both pass the bids-validator with 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Version bump to v1.0.20260724 for the development->master release. Pre-release audit fixes: document the -br flag in -h usage (was shipping undocumented), and guard the JPEG2000 multi-fragment magic sniff with size<=8 so a degenerate reassembled codestream cannot read past the allocation (mirrors the single-fragment branch). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
dcm2niix v1.0.20260724 (24-July-2026)
tools/mrs_post.pyfor vendor-state post-processing. Validation Dataset._noRFsuffix. Validation Dataset.MatrixCoilMode,PulseSequenceType, and (for PET)MolarActivityandAcquisitionTime.AcquisitionContrastas a vendor-agnostic fallback.