feat: rename BIDS files for multi-orientation series (rebase of #788 + fixes) - #869
Draft
yarikoptic-gitmate wants to merge 11 commits into
Draft
feat: rename BIDS files for multi-orientation series (rebase of #788 + fixes)#869yarikoptic-gitmate wants to merge 11 commits into
yarikoptic-gitmate wants to merge 11 commits into
Conversation
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
…arate niftis for the same series
BIDSFile.__str__ reconstitutes a filename from _known_entities, so anything that list gets wrong, or does not contain, comes out wrong. Three problems, all of which the chunk- renaming (which is the first production code path to feed real output basenames through parse/__str__) would have hit: - The list was incomplete and, with the entities added for the chunk- work, out of order: nuc/voi/trc/stain/tracksys were appended at the end while the BIDS entity table interleaves them among task-, acq- and ce-. Fill it in from the entity table (rules/entities.yaml of the BIDS schema), in the order it mandates, and add heudiconv's own non-standard `ch` at the spot update_uncombined_name() puts it, so the two agree. - parse() kept only the entities it knew about, so anything else was silently dropped on the round-trip: "sub-1_space-T1w_desc-x_bold.nii.gz" came back as "sub-1_bold.nii.gz". Keep such entities, placing them right before the suffix and warning that we cannot know where they belong. - parse() looked for key-value pairs anywhere in the name and assumed the last one was followed by the suffix. Both halves are wrong: reproin marks duplicated series with a trailing "__dup-01", so "sub-1_task-rest_bold__dup-01" lost its `bold` suffix entirely, and a "T1w-mod" suffix contains a "w-mod" pair. Match the leading run of pairs as a whole instead, and raise ValueError when there is none rather than IndexError.
update_multiorient_name() indexed the orientations by looking the file's ImageOrientationPatientDICOM up in the lexicographically sorted set of the orientations of the series. That is a sort of the *string* rendering of a list of floats, so the order is arbitrary: on the 3-plane localizer used by the tests it yields Sag/Cor/Tra as chunk-1/2/3 even though Tra was acquired before Cor. Worse, it is not stable across subjects -- any difference in obliquity or in float formatting reshuffles it, so chunk-1 could be the sagittal plane for one subject and the coronal one for the next, and for the multi-FoV BOLD case the two FoVs could swap between subjects. Keep indexing the orientations (so that files sharing an orientation share a chunk, which matters when the series is *also* split by echo or by magnitude/phase) but take their order from the order dcm2niix emitted them in rather than from sorting their values. Also stop the renaming from taking the whole conversion down with it: a basename with no entities at all, or without sub-, used to raise out of save_converted_files, and a sidecar missing ImageOrientationPatientDICOM while its siblings have it used to raise a KeyError. Warn and hand the name back unchanged instead, so the pre-existing fallback applies. Say so in the warning for the already-has-chunk- case too, which is otherwise silent about producing a mangled suffix. Also fix the copy-paste leftovers in the bids_localizer heuristic docstring, and note there that BIDS has no localizer/scout suffix.
The fixture is only used to check how the resulting files are named, so full 512x512 pixel data buys nothing while accounting for 76% of the 2.0MB the three files took -- 12% of heudiconv/tests/data. Decimate it 8x, scaling PixelSpacing to match so the geometry is preserved (2.0MB -> 508KB, in line with the other DICOMs we ship; what is left is mostly the Siemens CSA header, which dcm2niix needs). While at it, empty out the free-text InstitutionAddress / OperatorsName / ReferringPhysicianName fields; the subject is a phantom, so the remaining patient-level tags are of no concern.
…lliding dcm2niix splits a series on more criteria than we account for (matrix size, position, derived images), so a series can yield several files of which two share an image orientation. chunk- cannot tell those apart, and since the name *was* modified the pre-existing numeric fallback did not kick in, so we went on to move two files onto the same destination -- with --overwrite that silently loses one, without it safe_movefile raises and takes the whole conversion down. Before this series of commits the same input converted fine, as sub-X_localizer1..4. Work out all the output names first, and only use them if they came out distinct; otherwise fall back to appending the index to the suffix for all of the files, as we do when no renaming applies at all. Also hoist the iops lookup out of the try:, so that a ValueError from list.index() cannot be mistaken for an unparseable filename, and give the "orientation not among those of the series" case its own message instead of claiming the sidecar has no ImageOrientationPatientDICOM.
yarikoptic-gitmate
added
the
minor
Claude
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #869 +/- ##
==========================================
+ Coverage 83.44% 84.22% +0.78%
==========================================
Files 42 43 +1
Lines 4451 4799 +348
==========================================
+ Hits 3714 4042 +328
- Misses 737 757 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yarikoptic
marked this pull request as draft
August 6, 2026 20:05
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
3 participants
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.
Takes @bpinsard's #788 (which adds a
chunk-entity when dcm2niix splits one series into several images with different orientations, e.g. a 3-plane localizer), rebases it onto current master, and fixes four problems found reviewing it. The first two commits are #788 unchanged; the last four are the fixes. Supersedes #788 — please review there for the entity choice, here for the implementation.Without this, such a series is silently written out as
sub-X_T1w1,sub-X_T1w2, … — a mangled suffix, not valid BIDS.The four fixes
BIDSFiledid not round-trip filenames.__str__rebuilds a name from_known_entities, so whatever that list gets wrong comes out wrong, and #788 is the first production path to feed real output basenames throughparse/__str__.nuc/voi/trc/stain/tracksys) were appended at the end, but the entity table interleaves them amongtask-,acq-andce-. Filled the list in fromrules/entities.yaml, in the order it mandates, plus heudiconv's ownchwhereupdate_uncombined_nameputs it.sub-1_space-T1w_desc-x_bold.nii.gz→sub-1_bold.nii.gz. Now kept, before the suffix, with a warning.parselooked forkey-valuepairs anywhere and assumed the last one preceded the suffix. reproin marks duplicated series with a trailing__dup-01, sosub-1_task-rest_bold__dup-01lostboldentirely; aT1w-modsuffix contains aw-modpair. Now matches the leading run of pairs as a whole.The
chunk-index was arbitrary. #788 indexed orientations by looking the file'sImageOrientationPatientDICOMup in the lexicographically sorted set of the series' orientations — a sort of the string rendering of a list of floats. On the localizer fixture that makes Corchunk-2though it was acquired last, and it is not stable across subjects: any difference in obliquity or float formatting reshuffles it, sochunk-1could be the sagittal plane for one subject and the coronal one for the next, and the brain/c-spine multi-FoV BOLD case could swap the two FoVs between subjects. Now indexes the distinct orientations in file order, which is deterministic and still groups files that share an orientation (so a series split by echo or mag/phase as well gets one chunk per FoV, not per file).Two paths aborted the whole conversion. A basename with no entities, or without
sub-, raised out ofsave_converted_files; a sidecar missingImageOrientationPatientDICOMwhile its siblings have it raisedKeyError. Both now warn and leave the name alone.Colliding names. dcm2niix splits on more than orientation, so a series can yield two images sharing one.
chunk-cannot tell those apart, and because the name was modified the numeric fallback was skipped — two files then moved onto the same destination, whichsafe_movefileturns into aRuntimeError(or, with--overwrite, silent data loss). Output names are now computed up front and only used if they came out distinct.Also: shrank the new test fixture (2.0MB → 508KB; full 512² pixel data buys nothing for a filename test) and emptied its free-text
InstitutionAddress/OperatorsName/ReferringPhysicianName.Notes on where this stands w.r.t. BIDS and dcm2niix
chunkwas generalized from microscopy to MRI in BIDS 1.9.0 (bids-specification#1586) and its definition in 1.11.1 — "images of the same physical sample with different fields of view acquired in the same imaging experiment … may be used to indicate different anatomical structures or regions of the same structure" — describes this case exactly. It is allowed inanat,func,dwi,fmapandmicr; not inperf,petormrs, where these names will not validate.dcm2niix (checked with 1.0.20260724 from PyPI) offers nothing to align with: plain
-b ydisambiguates with a non-BIDS_i0000<N>postfix, and its own BIDS naming (-f %h/%H, both flagged experimental) appends that same postfix after the suffix. ItsBidsGuessfor this localizer is["discard", "_acq-fl2_run-1_localizer"], and BIDS still has nolocalizer/scoutsuffix at all — so the localizer use case is out of BIDS regardless, and the entity is really there for the other multi-FoV series.Known limitation, unchanged from #788:
is_multiorientcompares orientations as exact strings, so two files of the same plane differing in the last decimal would be read as multi-orient.An AI assistant (Claude Code) was used substantially for the four fix commits and their tests; the two commits from #788 are @bpinsard's. Full suite green (209 passed, 15 skipped), mypy and lint clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_015ZMpRTSaHJ3bnHFtSjehkT
Generated by Claude Code