Skip to content

Fix(io): formats bound to RAS/LPS validate their endpoints; a transformation is never relabelled as a format (#343) - #360

Merged
balbasty merged 5 commits into
mainfrom
claude/fix/343-refuse-relabel-into-formats
Oct 9, 2026
Merged

balbasty merged 5 commits into
mainfrom
claude/fix/343-refuse-relabel-into-formats

Conversation

@balbasty

@balbasty balbasty commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Since #315, t.to(SomeFormat) relabelled any family member as the format. For example, a Scaling from LPS to LPS became a NiftiVoxelToRAS that was still labelled LPS→LPS, and io.save wrote it.

This PR follows the decision in #343 and its review: no priority. Classes bound to semantic axes validate their endpoints, so a wrong relabel fails when the object is built. Explicit converters remain only where they bridge exactly, or where no validator can decide.

Validators

Each class bound to a system has an explicit __post_init__ that checks its endpoints with check_endpoint(t, side, expected), a new module-level helper in io/transformations/base/affines.py.

  • Classes: VoxelToRAS, RASToVoxel, VoxelToLPS, LPSToVoxel, RASToRAS (inherited by the NIfTI and NiftyReg affines), RASCoordinatesField, LPSCoordinatesField (inherited by NiftiRASCoordinatesField), RASToWarpField/WarpFieldToRAS, and FlirtTransform.

  • Compatible, not a subclass: an endpoint passes if it is compatible with the bound system. None, unknown axes and open systems are accepted.

  • ⚠️ Two allowances beyond CoordinateSystem.compatible_with, which otherwise rejects systems that are clearly the same:

    • Axis names are ignored. RASmm names its axes x/y/z while RASCoordinateSystem names them by orientation, and VoxelCoordinateSystem uses dim0… while FVoxelCoordinateSystem uses i/j/k.
    • A system with fewer axes is compared with the first axes of the template, so a 2-D ITK world in L/P matches.

    This is done in io by rebuilding the axes without their names. Your call: a name-insensitive option in the datamodel's compatible_with might be the better home.

  • The error is brainhops.errors.IncompatibleSystemError(ConversionError). Building a class directly with the wrong endpoints raises it. Because it is a ConversionError, t.to(...) also raises it (or returns False with error=False), and io.save lists it as a reason. convert needs no change.

Converters

Format t.to(Format)
NiftiVoxelToRAS / NiftiRASToVoxel Exact for every affine family, through affine_between: LPS is bridged into RAS, and unknown systems are taken as the format's. An LPS→LPS Scaling is still refused.
NiftyRegAffine Exact through RAS→RAS, now also for unknown systems. Voxel systems are refused.
VoxelToRAS, RASToRAS, … (not formats) Relabelled only if the endpoints are compatible, otherwise IncompatibleSystemError.
NIfTI fields, SPM Exact (#314, unchanged).
MatrixAffine (all containers) Relabel allowed: a bare matrix keeps the affine's own endpoints, so the map is unchanged. Txt→Csv is allowed.
ITK TFM/H5/MAT A chain may be relabelled; anything else is refused by the generic fallback.
FLIRT Refused: its matrix depends on the raw matrix and the two images.
LTA, NiftyReg fields, ITK NIfTI fields, FNIRT, M3Z, OME-Zarr Refused, citing #312. For each, a relabel would build a wrong object or one that cannot be built.
X5, elastix Refused by the generic fallback (as on main).
  • Kept:
    • the exact bridging converters for NIfTI and NiftyReg, with their family source keys, because the family converter doesn't flip LPS to RAS;
    • the Feat(io): convert general transformations exactly into the NIfTI and SPM formats (#120) #314 field and chain converters;
    • converters from a format to its own class, where a converter on the format's family would otherwise catch that case. They defer to the family rebuild (_convert_withlog, _convert_withsplines, smart_replace), never replace.
  • Removed: the refusals that the validators now make redundant, the refusals for MatrixAffine and ITK chains, and NiftyReg's refusal of undeclared systems.
  • Converting a format to its own class works for every format: .to(matrix=...), inverse() and .to(log=False). This fixes NiftyRegAffine(...).inverse(), .to(matrix=...) and .to(log=False), which raised on main.

⚠️ Behaviour changes to review

  • A plain Affine with no declared systems is now saved as NiftyReg to .txt, consistent with "unknown means the format's systems". Main's test_plain_affine_is_not_saved_as_niftyreg became test_plain_affine_is_saved_as_niftyreg, which also checks the round trip, and a new test_a_voxel_affine_is_not_saved_as_niftyreg is added. Say if NiftyReg should instead keep refusing undeclared endpoints.
  • MatrixAffine and ITK chains accept relabels, as described above.
  • io.save of a voxel→voxel transformation to .nii or .txt is refused; main wrote it relabelled.
  • Open: whether the FSL scaled-mm classes should get validators that know the image geometry later. Their systems depend on the image.

Tests

  • Restored: the [scaling] case of test_a_transformation_no_nifti_format_holds_is_refused, an LPS→LPS Scaling. It fails on main.
  • tests/test_io_into_formats.py (251 tests):
    • validators reject incompatible endpoints on direct construction (10 affine classes, both coordinates fields, FLIRT);
    • compatible endpoints are accepted (unknown, other names, unknown axes, open systems, 2-D LPS);
    • a relabel into a bound class raises ConversionError, and error=False returns False;
    • refusals across 12 families × 12 formats, the [FEAT] Exact converters into the remaining transformation formats (ITK, NiftyReg, LTA, M3Z, X5, Elastix) #312 refusals, and the FLIRT reason;
    • MatrixAffine and ITK relabels are allowed, and NiftyReg accepts unknown endpoints;
    • the exact bridges, a format converted to its own class, and variants of one format;
    • io.save refusals.

Checks

  • Full suite (Python 3.8): 5340 passed, 91 skipped, 1 xpassed. Main (56190e3): 5087 passed.
  • ruff check, ruff format --check and codespell (with CI's ignore-words file) are clean.
  • Main (56190e3) is merged into the branch.

Closes #343

🤖 Generated with Claude Code

https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8

claude added 3 commits October 9, 2026 14:15
…o a format (#343)

The generic family converters rebuild a transformation as whatever class
they are asked for, so `t.to(Format)` relabelled a Scaling, a Linear, a
chain... as a format whenever no converter of the format caught it, and
io.save wrote the result. Each format now has explicit converters for
every family that reaches it: exact ones for NIfTI (every affine family,
SVF and CartesianField) and NiftyReg (any affine between declared world
spaces, bridged to RAS), and `no_exact_conversion` refusals for FLIRT,
LTA, the bare matrices, ITK, ITK NIfTI fields, FNIRT, M3Z, NiftyReg
fields, X5, elastix and OME-Zarr until #312 adds exact ones.

A format converted to its own class goes through the family rebuild
(`_convert_withlog`, `_convert_withsplines`, `smart_replace`) instead of
`replace`, which fixes `.inverse()`, `.to(matrix=...)` and `.to(log=...)`
on a NiftyRegAffine and `.to(field=...)` on a NIfTI field of coordinates.
Another variant of a format (Txt to Csv matrix, LTA vox-to-vox to
RAS-to-RAS, TFM to H5) is refused rather than relabelled.

Restores the `[scaling]` case of test_io_save and adds
tests/test_io_into_formats.py.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…ead of refusing relabels (#343)

Classes bound to coordinate systems now check, in their own
`__post_init__`, that their endpoints are compatible with them
(`check_endpoint`, which compares as `CoordinateSystem.compatible_with`
does but leaves out axis names and compares a shorter system with the
first axes): VoxelToRAS, RASToVoxel, VoxelToLPS, LPSToVoxel, RASToRAS
(and so the NIfTI and NiftyReg affines), RAS/LPSCoordinatesField,
RASToWarpField, WarpFieldToRAS and FlirtTransform. They raise the new
IncompatibleSystemError, a ConversionError, so a relabel through the
data model's converters fails as `t.to(Format)` refusing it.

The refusals that only stopped relabels go: MatrixAffine (and its
containers) and ITK chains now hold a relabelled transformation with its
own endpoints, and the X5 and elastix converters were never reachable.
FLIRT keeps a refusal that says why (its matrix depends on its images),
as do LTA and the field formats that derive their systems from their
content, where a relabel builds a wrong object. The exact NIfTI and
NiftyReg converters stay, so that LPS endpoints are bridged; NiftyReg
now takes unknown endpoints to be RAS, as NIfTI does, so a plain Affine
is saved as NiftyReg (test updated).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
@balbasty balbasty changed the title Fix(io): refuse or convert exactly every transformation converted into a format (#343) Fix(io): formats bound to RAS/LPS validate their endpoints; a transformation is never relabelled as a format (#343) Oct 9, 2026
Comment thread src/brainhops/io/transformations/base/affines.py Outdated
Comment thread src/brainhops/io/transformations/base/conversions.py Outdated
Comment thread src/brainhops/io/transformations/base/conversions.py
claude added 2 commits October 9, 2026 16:58
Rename base/conversions.py to base/_conversions.py and stop exporting it
from the base package, so it leaves the API reference. Make the endpoint
check and the standard systems of base/affines.py private, and move them
below the classes. Docstrings that linked to them now describe the check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
…md (#343)

Rewrite the docstrings and comments that #360 adds or changes so they
follow the documentation rules in CLAUDE.md: complete sentences that
start with the purpose, no fragments or shorthand, fewer colons and
dashes, NumPy sections with typed parameters, and a 72-column wrap.
Define "relabelling" where the term is used. Move the "# io" section
comment in nifti/_converters.py back above the io imports. The syntax
trees are unchanged once docstrings are removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3KmPTJ2CihFMCFaJ4twx8
@balbasty
balbasty merged commit 7873edb into main Oct 9, 2026
5 checks passed
@balbasty
balbasty deleted the claude/fix/343-refuse-relabel-into-formats branch October 9, 2026 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

t.to(IoFormat) now relabels any family member as the format instead of refusing (since #315)

2 participants