Repository navigation
Conversation
#415) Pass 1c of #415. Every NIfTI transformation (the two affines, the RAS coordinates and displacement fields, SPM, NiftyReg, ITK NIfTI and FNIRT) now holds its header as NiftiMetadata and its array, as the file stores it, in `raw`. `data` is a cached view decoded by pure pairs in `io/common/nifti/_views.py`; decoded NumPy views are read-only and the writers always write `raw`, so untouched files are written byte for byte. NiftiReaderWriter, `_explicit_matrix`, `_store_through_the_parser`, `_WritableNifti` and `sniff_nibabel` are deleted, and the `image` and `header` attributes are gone. Modules nifti.{base,affines,fields} and spm.y become private (`_base`, `_affines`, `_fields`). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
The formats of other packages imported private helpers of io.transformations.nifti. `_record_image` and `_set_coordinates_intent` move to io.common.nifti._header, and `_record_image` now takes a copy of the NiftiRaw record, which NiftiBasedTransformation._record returns. The `unset` predicate `_always` becomes `always_unset` in brainhops._core.properties, documented with `unset=` of smartproperty. NiftyReg and SPM define their own setter of `data`, as FNIRT and ITK do. The re-application of `data=` after construction, which three NIfTI classes copied, is done once in NiftiBasedTransformation.__post_init__. Because the concrete formats list the data model first, Transformation.__post_init__ now hands on to a later __post_init__ in the method resolution order when one exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
The writer of a NIfTI affine stored the sform and the qform under one code, so a file with sform code 1 and qform code 0 was saved with 1 and 1. Each form now keeps the code that the header records. The sform takes the qform code when its own code is zero, since it always holds the matrix, and a header without codes still gives code 2 to both forms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
balbasty
left a comment
There was a problem hiding this comment.
In general. We need to harden smartproperty so that we don't need these calls with lots of options, or ugly setters that do their own invalidation.
| # data model holds them with shapes (X, Y, 2) and (X, Y, Z, 3). | ||
|
|
||
|
|
||
| def _itk_field_to_model(raw: ArrayProtocol) -> ArrayProtocol: |
There was a problem hiding this comment.
Thos should be under itk something
There was a problem hiding this comment.
Moved in c7eb27a to io/transformations/itk/nifti/_views.py. It lives under itk/nifti rather than itk, because it needs nibabel and only the ITK NIfTI fields use it.
Generated by Claude Code
| # holds the same field with shape (X, Y, Z, C). | ||
|
|
||
|
|
||
| def _field_to_model(raw: ArrayProtocol) -> ArrayProtocol: |
There was a problem hiding this comment.
This should probably be under io.transformations.something.
And the image equivalent under io.images.something
There was a problem hiding this comment.
Moved in c7eb27a. All three new modules are private, and io/common/nifti/_views.py is deleted:
- the field views and the affine views now live in
io/transformations/nifti/_views.py; - the image views now live in
io/images/nifti/_views.py.
The shared _read helper stays in io/common/nifti/_files.py. When the change is merged up the stack, the image views of MGH, AFNI, MRtrix, NRRD and MINC will follow the same rule.
Generated by Claude Code
| infer it from the data. Coefficient fields ignore it. | ||
| """ | ||
|
|
||
| @smartproperty(cache=True, unset=always_unset, fset=_set_fnirt_data) |
There was a problem hiding this comment.
Isn't this sort of equivalent to a lazyproperty?
There was a problem hiding this comment.
Yes. It is now a @lazyproperty with a @data.setter, and always_unset is gone (b033ff6). The same change applies to every NIfTI-based data view: the affines, the coordinates and displacement fields, SPM, NiftyReg, ITK-NIfTI and FNIRT.
lazyproperty now accepts invalidates= and a setter (80efb0d). Its unused unset= is removed.
Generated by Claude Code
| def to_nibabel(self, like: tx.Any = None, **overrides) -> tx.Any: | ||
| """Build the NIfTI image while preserving the FNIRT header intent.""" | ||
| if self.data is None or self.header is None: | ||
| """Build the NIfTI image of the field under the header of its file. |
There was a problem hiding this comment.
Always document public methods in numpy style (with Parameters section)
There was a problem hiding this comment.
Fixed in c7eb27a, and applied to every public method changed in this PR. That covers:
- each
to_nibabel(FNIRT, the NIfTI fields and affines, ITK-NIfTI, NiftyReg, SPM); - the readers and writers of
NiftiBasedTransformation; - the
inversemethods; - the
from_anyandfrom_instanceoverrides.
They all have Parameters, Returns and, where needed, Raises sections.
Generated by Claude Code
| so that a copy made with `replace` takes the matrix from `raw`. | ||
| """ | ||
| self.raw = None if value is None else _affine_to_disk(value) | ||
| self._data = None |
There was a problem hiding this comment.
Self.data = None also clears the cache implicitly, no?
There was a problem hiding this comment.
Inside the data setter, self.data = None would call the setter again. The line self._data = None does something else: it empties the private field in which the data model stores its array, not the cache. That has two effects:
- the getter never serves the undecoded array;
replaceandfrom_instancecopy the transformation lazily, fromraw.
The hand-written cache pop is gone, because every setter now drops its own cache (b033ff6, 80efb0d). A comment at the line explains why _data is emptied.
Generated by Claude Code
| the metadata, so other metadata gives another matrix. | ||
| """ | ||
| self._metadata = value | ||
| self.__dict__.pop("_cache_data", None) |
There was a problem hiding this comment.
Same as making the property invalidating data?
There was a problem hiding this comment.
Yes. invalidates= now takes names, invalidator objects, or a tuple mixing both (80efb0d). The setter became metadata = smartproperty("metadata", invalidates=("data", _FORGET_VIEWS)), and _set_affine_metadata is gone (b033ff6).
Generated by Claude Code
| # anything else is read or copied as the bases do. | ||
|
|
||
| @classmethod | ||
| def from_any(cls, other: tx.Any, *args, **kwargs) -> tx.Self: |
There was a problem hiding this comment.
I feel like something like this should be implemented at a more fundamental level, so we don't have to rewrite it in each class. Might also be the case for some of the other similar methods.
There was a problem hiding this comment.
I tried this and held it back, because it changes tested behaviour. The attempt converted on TransformationFormat.from_any/from_instance whenever a converter exists, and it hit two problems.
Behaviour change. Today, a format with no converter for its input relabels the object as that format. Converting at the base turns that relabelling into a ConversionError for LTA, FLIRT, FNIRT, ITK-NIfTI, NiftyReg fields, Zarr and M3Z:
- 7 tests in
test_io_lta_format.pyfail, because saving anAffineas LTA relies on the relabelling; - 2 NiftyReg velocity tests fail as well.
LTA's own converters refuse every input until #312, so I found no rule that keeps today's behaviour.
Method resolution order. In the NIfTI affines and the coordinates field, ConcreteTransformation.from_instance comes before TransformationFormat. It decodes the data first, so those overrides would have to stay anyway.
My suggestion is to keep the explicit per-class copies until exact converters land (#312), then move the logic to the base. The alternative is to accept the stricter behaviour now. Which do you prefer?
Generated by Claude Code
There was a problem hiding this comment.
Done with the fallback you approved, without any converter priority (40e792f, merged up through #427). t.to(Fmt), Fmt.from_any(t) and Fmt.from_instance(t) are now one conversion for every format.
The fallback. relabelling(t, cls) is a single converter function in io/transformations/base/_converters.py, built on a shared relabel helper.
- It rebuilds
tas the format, keeping its parameters. - It refuses with
ConversionErrorwhen the family does not match, or when a known endpoint fails_check_endpoint. For example, an LPS affine is never relabelled as a RAS-to-RAS LTA. - Formats with no exact converter register it explicitly per family, where the old
no_exact_conversionrefusers were (removed):- LTA: its 9 affine families and
Sequence; - NiftyReg, FNIRT, ITK-NIfTI and Zarr: chains;
- M3Z and X5: chains (in Refactor(io): M3Z holds an M3zRaw record through M3zMetadata (#415, pass 2c) #423 and Refactor(io): X5 holds an X5Raw record through X5Metadata (#415, pass 2b) #422).
- LTA: its 9 affine families and
- The NIfTI exact converters are more specific, so they still win.
Once on the base. from_any and from_instance now live on TransformationFormat. The per-class copies are removed: the NIfTI affines and fields, SPM and NiftyReg. Two overrides stay, each narrowed and calling the shared helpers:
- the NIfTI affines and the coordinates field, because
ConcreteTransformation.from_instanceprecedesTransformationFormatin their MRO; - LTA's within-format
from_instance.
Visible changes:
.to(LtaTransform)and the other [FEAT] Exact converters into the remaining transformation formats (ITK, NiftyReg, LTA, M3Z, X5, Elastix) #312 formats now relabel compatible transformations. Before, they refused, whilefrom_anyalready relabelled.from_instancewith the wrong family now raisesConversionError. Before, it built a meaningless object.io.savenow picks a format only through an exact conversion. A relabel would make it ambiguous, because a chain is accepted by NiftyReg, FNIRT and ITK-NIfTI alike.- Elastix and ITK containers keep refusing chains through all three entry points.
Generated by Claude Code
| EXTENSIONS: tx.ClassVar[tx.Tuple[str, ...]] = (".nii", ".nii.gz") | ||
| HINTS: tx.ClassVar[tx.Tuple[str, ...]] = ("nifti",) | ||
|
|
||
| raw: KwOnly[NoRepr[tx.Optional[ArrayProtocol]]] = None |
There was a problem hiding this comment.
I've said it before, but we really need kw_only=True by default, and add NoKwOnly on fields that we want positional (the ones most likely to be passed on init)
There was a problem hiding this comment.
Transformations already run with kw_only=True: it is set on Transformation and TransformationFormat, and bagof Magic inherits the option. So the KwOnly[...] wrappers on raw and _metadata here, and in LTA, X5 and M3Z, are redundant. They will be removed in the PRs that add them. The comment in the signature test saying the option is "not inherited" is also wrong, and will be fixed. bagof already has NotKwOnly, so no bagof change is needed.
Images do not have the option yet. I propose a small separate PR on main:
- set
kw_only=TrueonImage,ImageFormatandMultiScaleImage; - keep
NotKwOnlyondata,transformationsandimages; - pin every public image signature in the signature test.
That PR changes the reader fields of the openslide, TIFF, Pillow and Zarr classes, about 22 classes in total, from positional to keyword-only. The PR would list them.
A repo-wide default on DataModelBase would also touch about 60 axis, system and orientation classes, so I'd treat it as its own decision. Should I open the image PR, and do you want the repo-wide default as well?
Generated by Claude Code
There was a problem hiding this comment.
Following your answer (transformations and images only, not axes), the transformation side is done in c7ac849:
- the redundant
KwOnly[...]wrappers onrawand_metadataare removed; - all 18 NIfTI-based classes keep identical signatures;
- the signature test comment is fixed;
- a small test guards the inheritance of
kw_only.
LTA, X5 and M3Z follow when the change is merged up the stack. The images get kw_only=True in a separate small PR on main.
Generated by Claude Code
| # stored again, through the setter of `data`, which writes `raw`. | ||
| # When both are given, `data` takes precedence over `raw`. A | ||
| # format that takes no `data=` is not affected. | ||
| if arguments.get("data") is not None: |
There was a problem hiding this comment.
This seems to be necessary in many places. Can we inherit that behaviour? Or at least have a helper? Or implement something more general in bagof?
There was a problem hiding this comment.
Added as a helper, not as inherited behaviour, to keep it explicit. store_data_argument(obj, arguments) is a module-level function in io/base/_base.py (1b420aa). Each __post_init__ is now one call to it, here in NiftiBasedTransformation (6838c6c) and in NiftiImage (eded01e). LTA and the later images switch to it when the change is merged up the stack.
A bagof option that assigns explicit arguments after the defaults would remove the need for the helper entirely. That would be a bagof change for you to consider; nothing here depends on it.
Generated by Claude Code
…y docstrings (#415) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…forms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8 # Conflicts: # src/brainhops/_core/properties.py
… setters (#415) The hardened smartproperty drops the cache of a property on every setter, so the module-level _set_*_data helpers, their cache pops, and always_unset are gone. Each data view is a lazyproperty with a setter that stores raw; the affines and the coordinates field keep emptying the model's _data field, so that the view never serves what the constructor stored and replace() reads raw. The affine metadata invalidates ("data", _FORGET_VIEWS) instead of using its own setter, and metadata invalidates "transformations" given as a string. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…rgument (#415) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…ions (#415) TransformationFormat declares kw_only=True, which bagof Magic passes on to subclasses, so raw and _metadata are keyword-only without KwOnly[...]. The signatures are unchanged. The signature test no longer says that the option is not inherited, and a test guards the inheritance. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…forms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…rs; from_any once on TransformationFormat (#415) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…version (#415) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…forms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…er family, without a priority (#415) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…forms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…forms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…forms Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
Pass 1c of #415. It is stacked on #418 (pass 1b, the NIfTI image), which is stacked on #417 (pass 1a, the core). Design: #416 (
docs/design/format-records.md).Every NIfTI transformation now holds a record of its file instead of inheriting
NiftiReaderWriter. That covers the two affines, the RAS coordinates and displacement fields, SPM, NiftyReg, the ITK NIfTI fields and FNIRT. Each one holds:metadata, aNiftiMetadatawrapping the header record;raw, its array as the file stores it, usually nibabel's lazy proxy.datais a cached view ofraw, decoded by pure functions inio/common/nifti/_views.py.What changes
NiftiBasedTransformationdeclaresrawand a private_metadatafield. It exposesmetadataas asmartpropertythat clears the decoded views, re-appliesdata=once in__post_init__, and reads throughread_nifti.Transformation.__post_init__now calls the next__post_init__in the MRO when there is one. The concrete formats list the model first, so the format's hook would otherwise never run.rawunder a copy of the record and keeps the record's forms. A NIfTI affine keeps the code of each form, so sform 1 / qform 0 is written back as 1/0.io/common/nifti/_header.py:_record_image,_set_coordinates_intent,_header(obj), and the structural reset and stored-scaling helpers.always_unsetmoved to_core/properties.py. Other packages now import only the publicNiftiBasedTransformationfromio.transformations.nifti.NiftiReaderWriter(_parsers.py),sniff_nibabel,_explicit_matrix,_store_through_the_parser,_WritableNifti,_files._load_nifti*, and theimage/headerattributes.Visible behaviour changes
raw(lazy proxy) andmetadata(NiftiMetadata); removed:image,header,image=/header=arguments,sniff_nibabel,NiftiReaderWriter,_WritableNifti;from_nibabeltakes an image or header only.nifti.affines/base/fields→ private;spm.y→brainhops.io.transformations.spmexportsSpmCoordinatesField;niftipackage re-exportsNiftiMetadata,NiftiRaw;fnirtandniftyregpackages have pinned__all__.header=becomesmetadata=.scl_*, description, intents and NiftyReg extension affines; a NiftyReg velocity with extensions is written back as read.DISPVECTfile read and saved unchanged staysDISPVECT(RAS); only a field with an assigned chain is written asVECTORLPS.data/matrixto change them.inverse()of a header affine stays header-backed; assigningmetadatadrops decoded views.datais now(X, Y, Z, 3)(singleton axis dropped); positional constructor parameters changed (seePOSITIONAL).log=Trueare written back as velocity unlesssave(log=False).Tests
EXEMPLARS.Exemplar.buildhandles constructors that need more thandata=, and a dedicated byte-identity test covers the unregisteredNiftiRASToVoxel.io.save. That covers FNIRT (2006 and 2007), NiftyReg control-point grids and velocities with extension affines, SPM, ITK (including 2-D) and displacement fields loaded withlog=True.POSITIONALin the constructor-signature test, plus the affine-inverse, registry, immutable-formats, NiftyReg, ITK, FNIRT, SPM and FSL tests.Review
Fable reviewed this PR and found no behavioural bugs. Its two fixes are in 7ac5981 (helper placement, a single
__post_init__), and its optional fix is in 01a6565 (each form keeps its code). The tensions found are logged in #416, items 33–38, with updates to 1, 5, 8, 12, 13, 18, 20, 21, 24 and 28.Checks
/tmp, Python 3.8): 5685 passed, 95 skipped, 1 xpassed.ruff check,ruff format --checkand codespell are clean.Part of #415.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
Generated by Claude Code