Repository navigation
Refactor(io): NIfTI images hold a record and a lazy raw array (#415, pass 1b) - #418
Conversation
NiftiImage no longer inherits NiftiReaderWriter. It holds the header of its file as NiftiMetadata, whose record is a NiftiRaw (the nibabel header with its extensions), and the voxels as stored in `raw`, a nibabel ArrayProxy after a read. `data` and `transformations` are cached views of `raw` and of the record. - io/common/nifti: new NiftiRaw (header-only read and write, sniffing moved from NiftiReaderWriter), NiftiMetadata (registered metadata format, never reads the voxels), pure view functions, and the read_nifti/write_nifti helpers. _image_with_geometry takes the record as the base header: structural fields are reset, the geometry is encoded from the model, like= and overrides come last, and an untouched proxy is written unscaled with the stored data type and scaling, so an untouched save is byte for byte. - NiftiImage drops `image`, `header`, `sniff_nibabel` and `_decoded`. NiftiReaderWriter stays, unchanged, for the NIfTI transformations. - The conformance tests run all twelve checks on NiftiImage, for .nii and .nii.gz, plus byte-identical copies of scaled files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
- Data decoded from a NIfTI proxy is a read-only NumPy array, and NiftiImage.to_nibabel always writes `raw`. Voxels are changed by assigning `data`, so an image that is only read stays byte for byte. - NiftiRaw.to_filename and NiftiMetadata.to_filename compress a header written to a path whose name ends with `.gz`. - NiftiImage.sniff_fileobj parses the header once. - The FSL geometry helpers look the header up once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
|
|
||
| _metadata: KwOnly[NoRepr[tx.Optional[NiftiMetadata]]] = None | ||
|
|
||
| metadata = smartproperty("metadata", invalidates=("transformations",)) |
There was a problem hiding this comment.
I think invalidated can be a string (not in a tuple), no?
There was a problem hiding this comment.
Yes. A string was already accepted, so only the call sites changed. They now read invalidates="transformations" (eded01e). invalidates= also takes an invalidator object, or a tuple mixing names and invalidators (80efb0d).
Generated by Claude Code
| if arguments.get("data") is not None: | ||
| self.data = arguments["data"] | ||
|
|
||
| @smartproperty(cache=True, invalidates=("data",)) |
There was a problem hiding this comment.
Does it make sense to invalidate itself? I thought all smart properties cleared their cache when assigned a new data by a user.
There was a problem hiding this comment.
You were right for the default setter, but not for a setter given with fset= or @x.setter. Those did not drop their own cache, which is why data named itself here.
This is fixed in smartproperty (80efb0d): every setter now drops the property's own cache before it runs. data is now a plain @lazyproperty with a setter, and no longer invalidates itself (eded01e).
Generated by Claude Code
| def data(self, value: tx.Optional[ArrayProtocol]) -> None: | ||
| self.raw = None if value is None else _image_to_disk(value) | ||
|
|
||
| @smartproperty(cache=True, unset=(None, "empty")) |
There was a problem hiding this comment.
I really don't like this unset=empty thing, seems so fragile (what if I actually want no transformations?). Maybe we should go back to allowing None transformations? Discuss.
There was a problem hiding this comment.
Agreed that unset="empty" is fragile, and it is also wrong today. Assigning [] to a loaded NiftiImage is silently ignored: the getter reads [] as unset and decodes the header again. As a result an image can never drop its geometry, which breaks the "honoured or refused" rule of the memo.
The root cause is the data model. Its default transformations = () is written through the format's setter by the constructor, so the format cannot tell the default apart from an empty list someone chose on purpose.
Proposal: allow None in the data model, the way Transformation already stores _input and _output.
SingleScaleImagegets_transformations: Optional[List] = None, behind atransformationsproperty that returns[]when nothing is stored.- Formats use a plain
@smartproperty(cache=True). Nonemeans "decode from the record".[]means "no geometry" and is written as such.replace()stays lazy, because it copiesNone.
There is one explicit consequence, in _FileBasedModelMixin.from_instance:
- Within a format it passes
transformations=None, because the record carries the geometry, asrawcarries the data. - Across formats it passes the decoded list.
The constructor signature is unchanged: transformations stays positional. The model part would go in #417, and the format part in #418 and the later image PRs.
I'm holding this one until you confirm. The other comments are being addressed now.
Generated by Claude Code
There was a problem hiding this comment.
Decision recorded: None only on the classes that implement a property on top of it, i.e. the formats. The data model is unchanged.
That idiom cannot be written yet, because of bagof:
- A format that redeclares
transformations: Default[Optional[List], None]beside@smartproperty def transformationsgets the property itself as the field default. bagof treats a class attribute as the default, as dataclasses do (magic/_magic.py:1965). - A private
_transformationsfield is refused, because it shares the public name of the model'stransformations.
The NIfTI transformations need nothing: their sequence bases already store _transformations: Optional = None. So NiftiImage keeps unset=(None, "empty") until one of these is decided:
- (a) A bagof change: when an annotation gives an explicit default, use it rather than a property of the same name defined on the class. Recommended.
- (b) The readers pass
transformations=Noneexplicitly. ThenNiftiImage(metadata=md)built by hand would no longer decode its geometry.
Generated by Claude Code
There was a problem hiding this comment.
Done with bagof-magic 0.3.dev3 (520c2d7 here; merged up the stack, with the other image formats in #424, #425 and #427). Each format that decodes its transformations takes the model's field over privately:
_transformations: Optional[List[Transformation]] = Inherit("transformations", default=None)
@smartproperty(cache=True)
def transformations(self): ... # None decodes from the record; a stored list, even [], is used as isThis applies to NiftiImage, MghImage, AfniImage, MrtrixImage, NrrdImage and MincImage. The data model is unchanged, and so is the signature (data, transformations, *, raw, metadata).
[]is honoured. It is kept on the image and on its copies, and written with the same geometry as an image built from bare data. In Refactor(io): NRRD and MINC images hold a record and a lazy raw array; saves over a read file keep its data (#415, pass 3c) #427, the NRRD writer no longer brings back the old spacings for[].Nonedecodes from the record.replaceandfrom_instancewithin a format stay lazy.- Copies to the model or to another format carry the decoded list.
Conformance checks 13 and 14 cover this on every image exemplar. unset="empty" remains only in OmeZarrImage, which takes its fields over the same way next.
Generated by Claude Code
…; invalidates by name (#415) The data view is a lazyproperty with a setter, which drops its own cache, so it no longer names itself in invalidates. metadata invalidates "transformations" given as a string. __post_init__ uses store_data_argument. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…ecodes from the record (#415) `NiftiImage` stored its transformations through a property over the inherited `transformations` field, and read an empty list as unset (`unset=(None, "empty")`), because the constructor writes the empty default of the field through the setter. An empty list assigned on purpose was therefore ignored, and the header geometry was written again. The image now takes the field over with `Inherit("transformations", default=None)`: the constructor still takes `transformations` in the same place, `None` decodes the header, and any list, even an empty one, is used as it is. An empty list is written as an image built from data alone is, with the identity under the code `aligned`. Code that compares a format with its data model now uses the public names of the fields: `_holds` in `io.save` and `_foreign_format_fields` in `from_instance`. Within the format, `from_instance` and `replace` copy the stored value, so a copy of an untouched image stays lazy. From another class, `from_instance` copies the transformations that the other object reports, decoded if need be. The conformance checks gain two tests that every exemplar runs, and the toy format follows the same idiom. A loaded image no longer shows its decoded transformations in its repr, since nothing is stored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…xemplars only (#415) The two conformance checks of the transformations of an image are parametrized over `IMAGE_CASES`, the cases whose format is an image, so that the transformation formats that later join the exemplars are left out. The check of copies from another class builds its own image of another format, and the NIfTI docstring uses the short cross-reference to `SingleScaleImage`, which the module imports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
… is gone (#415) `NiftiImage` takes `data` over as `_data`, so an assigned array is pending and `raw` keeps the proxy; `store_data_argument` and its `__post_init__` are gone. `consolidate(inplace=False, like=None, **overrides)` encodes the pending data, geometry and writer options into a copy with a new `raw` and a new record, and shares both for an untouched image. `to_nibabel`, `to_bytes`, `to_fileobj` and `to_filename` write the consolidated copy and leave the image as it was. The encoder keeps an untouched proxy, and `_written_nifti` writes a record as it is, unscaling a proxy that the header stores as it is, so an untouched file is copied byte for byte. `NiftiMetadata.to_raw` is removed; callers copy the record with `NiftiRaw.copy`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…t its file holds (#415) An override of the data type or of the scaling made the header store the voxels differently from `raw`, which still held the unconverted array, and the header had no scaling yet, since nibabel chooses it only when it writes. The encoded image is now written to bytes and read again in that case, so `raw` is an in-memory proxy of the stored voxels and the record holds the header that was written. The NIfTI exemplar gains the `dtype` storage case. `consolidate` takes `inplace` by keyword only, and the docstring of `transformations` no longer says that a transformation appended in place is kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…#415) The image stores the transformations that it decodes from the header on first access, so a change made to the list in place, such as an appended transformation, is kept. `consolidate` and the writers compare the list that the image holds with a fresh decode of the header, in the data model and exactly: when they are the same, or were never read, the geometry fields of the header are kept as they are stored, and the image is left as it is when nothing else is pending. Assigning metadata drops the list. The encoder reads the list without storing it, so that a write leaves the image as it was. Check 13 and two NIfTI tests now expect a read list to be stored, and an equal list to keep the header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…image Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…ten (#415) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
`NiftiImage` keeps the base list of this branch, which no longer starts with a file reader: its first bagof base is `ImageFormat`, which is keyword-only, so the repeated `kw_only=True` and its comment are dropped, and so are the `KwOnly[...]` wrappers of `raw` and `_metadata`. The signature stays `(data, transformations, *, raw, metadata)`, which the `IMAGES` table now pins. The docstring of `SingleScaleImage` gives the Zarr `node` as the example of a field that a reader adds, since NIfTI no longer has one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
Pass 1b of #415. It is stacked on #417 (pass 1a), so the base is
claude/feat/415-core-records. Design: #416 (docs/design/format-records.md).NIfTI images now hold a record of their file instead of inheriting
NiftiReaderWriter.What changes
brainhops.io.common.niftigains two public classes and a private module:NiftiRaw: the header record, with extensions. It reads and writes the header only, sniffs files, and itsto_filenamegzips on.gz. It is built on the module helpersread_nifti, which returns the record and a lazy proxy, andwrite_nifti.NiftiMetadata(MetadataFormat): a registered metadata format holding aNiftiRaw.NiftiMetadata.load(path), likeMetadataFormat.load, reads only the header and never touches the voxels._views.py: the image'sto_model/to_diskpair. A NumPy array decoded from a file is read-only.NiftiImageholds ametadata(aNiftiMetadata) andraw, the data as stored: nibabel'sArrayProxyafter a load, or an array once data has been assigned.datais a cached view ofraw, andtransformationsis a cached view of the header, cleared whenmetadatais reassigned.like=and the overrides are applied. The header is written first, then the data. An untouched proxy is written unscaled with the record's dtype andscl_*, so the file is copied exactly.Visible behaviour changes
descrip,aux_file, the intent,cal_*,slice_*,toffset,dim_infoand the extensions survive a save, and NIfTI-2 files stay NIfTI-2. Before, each save started from a fresh header.io.save.mm.datato change voxels; an in-place edit raises instead of being silently lost.img.transformationsis the same cached list each time, so in-place appends persist.NiftiImage:imageandheader, nowrawandmetadata.raw.header;sniff_nibabel, nowNiftiRaw.sniff_*;image=/header=constructor arguments;system, which is now read-only.like=accepts aNiftiImage, and the FSL FLIRT/FNIRT geometry reads its record.NiftiReaderWriterstays exported, for the NIfTI transformations, until pass 1c.Tests
NiftiImageandNiftiMetadatamove into the conformanceEXEMPLARS. All 12 checks pass on.niiand on.nii.gz.int16file withscl_slope(both suffixes, and throughio.save), a NIfTI-2 file, header-only reads of a truncated file, gzipped metadata, and read-only data.Review
Fable reviewed this PR, and its fixes are in e02244f: read-only views with the proxy always written, gzipped metadata on
.gz, and single header parses. The tensions found are logged in #416 (items 24–32 and updates to 4, 6, 13 and 18).Checks
/tmp, Python 3.8): 5513 passed, 95 skipped, 1 xpassed.ruff check,ruff format --checkand codespell are clean.Part of #415.
Changes after review
After the first review, the stack gained these changes. They are merged into every PR from #417 up, and the design note (#416) is updated to match.
smartpropertyis hardened: every setter drops its own cache, andinvalidates=takes names or invalidators.from_anyis defined once onTransformationFormat. One relabelling converter is registered explicitly for each format family; there is no converter priority.KwOnlymarkers on transformations are removed._transformations = Inherit("transformations", default=None).Nonedecodes from the record, and[]is honoured.Forgetreplaces theClassVarworkarounds, inherited defaults usex: ... = v, andunset="empty"is gone.consolidate().dataortransformationsnever touchesrawormetadata.consolidate(*, inplace=False, **options)is the only public encoding API.to_raw()is removed.save,to_bytesandto_fileobjwrite a consolidated copy and leave the object unchanged. The exception is a save over the file the object itself reads (Saving an uncompressed NIfTI or MGH image over its own file destroys its data #426), after which the object equalsload(path).The conformance harness gained checks 15–19, which cover consolidation, saving over the object's own file, storage overrides, and the transformations list.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
Generated by Claude Code
Generated by Claude Code