BF: Preserve MGH tag data across an unmodified round-trip (gh-1402) - #1540
Open
CedricConday wants to merge 1 commit into
Open
BF: Preserve MGH tag data across an unmodified round-trip (gh-1402)#1540CedricConday wants to merge 1 commit into
CedricConday wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1540 +/- ##
==========================================
+ Coverage 95.48% 95.53% +0.04%
==========================================
Files 209 209
Lines 30050 30144 +94
Branches 4494 4510 +16
==========================================
+ Hits 28694 28798 +104
+ Misses 925 914 -11
- Partials 431 432 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Same as on #1539 — Not from this diff, which doesn't touch packaging. Happy to rebase whenever it's convenient. |
FreeSurfer writes optional, variable-length tag data after the MGH footer (TAG_CMDLINE history, auto-align matrices, embedded colour tables, ...). `MGHHeader.from_fileobj` reads exactly `footer_dtype.itemsize` bytes and `writeftr_to` writes back only that struct, so everything following the footer was silently dropped on load/save. Round-tripping the bundled nibabel/tests/data/test.mgz loses 22431 of its 23215 bytes. The resulting file compares as having an identical header while being smaller on disk, and downstream FreeSurfer tools that read the tags (e.g. recon-all) fail on it. Retain the trailing bytes on read and write them back after the footer. The contents are not interpreted. Tags are not portable between volumes: TAG_MRI_FRAME carries per-frame state sized by the frame count and TAG_AUTO_ALIGN holds a geometry-specific transform, so emitting them alongside a different volume would produce a file FreeSurfer misparses. The bytes are therefore recorded with the header state they were read with and written back only while the header is unchanged, so deriving an image (`slicer`, `four_to_three`, a new shape or data type) drops them rather than corrupting them. The read is bounded by MAX_TAG_BYTES so that a file whose `dims` field understates the data -- which would make the voxel data itself look like tag data -- cannot pull an unbounded amount into memory; such a file warns and keeps no tags. The bound is deliberately generous, as TAG_MRI_FRAME grows with the frame count. `writeftr_to` now truncates, so writing a smaller image over a reused stream no longer leaves bytes from the previous image to be read back as tags. `ImageOpener` gains a `truncate` passthrough for that. Closes nipygh-1402 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
effigies
force-pushed
the
bf/mgh-preserve-tags
branch
from
August 24, 2026 20:18
266c072 to
56918f3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1402.
Problem
FreeSurfer may write variable-length tag data after the MGH footer —
TAG_CMDLINEhistory,TAG_AUTO_ALIGN,TAG_MRI_FRAME, embedded colourtables.
MGHHeader.from_fileobjreads exactlyfooter_dtype.itemsizebytes andwriteftr_towrites back only that struct, so everything after the footer issilently dropped on a load/save round-trip.
This is reproducible with the file already in the repo:
That file's tags are
[(41, 7), (42, 22400)]—TAG_PEDIRandTAG_MRI_FRAME.It presents confusingly, which is probably why the original thread stalled: the
header compares as identical in both nibabel and
mri_info, and only the filesize differs. That is exactly what the reporter described — "Both nibabel and
FreeSurfer's mri_info says that both images have identical headers. However
after decompressing the mgz to mgh I noticed the size of both files are not the
same." — and it is consistent with their
recon-allfailure.Fix
Retain the trailing bytes on read and write them back after the footer. They are
not interpreted.
Tags are not portable between volumes.
TAG_MRI_FRAMEstores per-frameacquisition parameters — 11200 bytes per frame in the bundled file — and
TAG_AUTO_ALIGNholds a geometry-specific transform. Writing them next to adifferent volume produces a file FreeSurfer will misparse: it would read
per-frame state for frames that no longer exist. So the bytes are recorded
together with the header state they were read with, and written back only while
that header is unchanged:
Editing voxel data while leaving the header alone — the case in the issue — keeps
the tags.
Two supporting changes:
MAX_TAG_BYTES. A file whosedimsfield understatesthe data makes the voxel data itself look like tag data; unbounded, a 597 KB
.mgzcan expand to over a gigabyte resident, on what is otherwise a lazyheader read. The bound is deliberately generous (64 MiB) because
TAG_MRI_FRAMEgrows with the frame count — a long BOLD run legitimatelycarries megabytes of tags. Over the bound, it warns and keeps none.
writeftr_totruncates. Writing a smaller image over a reused stream (e.g.to_file_mapwith aBytesIO) previously left bytes from the larger imagebehind; harmless before, but they would now be read back as tag data and
become sticky.
ImageOpenergains atruncatepassthrough for this.Tests
test_tag_data_roundtrip— byte-exact round-trip; tags survive a data-onlyedit; tagless files are untouched.
test_tag_data_not_reused_across_volumes— uses the bundledtest.mgz, andasserts tags are dropped for
four_to_three, a reshape and a dtype change.test_tag_data_bounded— over-long trailing region warns and keeps no tags.test_tag_data_not_laundered_from_reused_stream— the truncation case.Verified against
master: the round-trip above loses 22431 bytes there and 0here. Full suite 5290 passed, 270 skipped;
nibabel/freesurferis also greenunder
--parallel-threads 16.One judgement call worth flagging: because the fingerprint is the header, editing
only voxel data keeps the tags. That is right for
TAG_MRI_FRAMEandTAG_CMDLINE, but a segmentation whose labels are rewritten in place would keepa colour table that no longer matches. Distinguishing those needs the tags to be
parsed, which is a larger change than this. Happy to go further if you'd prefer.
AI-assisted, human-reviewed.