Skip to content

Fix TypeError thrown when specifying interpolator for ANTsRegistrator#178

Open
raph-rc wants to merge 1 commit intoBrainLesion:mainfrom
raph-rc:specify-ants-interpolator
Open

Fix TypeError thrown when specifying interpolator for ANTsRegistrator#178
raph-rc wants to merge 1 commit intoBrainLesion:mainfrom
raph-rc:specify-ants-interpolator

Conversation

@raph-rc
Copy link

@raph-rc raph-rc commented Mar 25, 2026

interpolator was passed twice to ants.transform when it was specified as an additional argument in
ANTsRegistrator(transformation_params={'interpolator': 'value'}). This change makes sure to pass it only once, and that the specified value takes precedence over the default. Fixes #177

`interpolator` was passed twice to `ants.transform` when it was
specified as an additional argument in
`ANTsRegistrator(transformation_params={'interpolator': 'value'})`. This
change makes sure to pass it only once, and that the specified value
takes precedence over the default.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a TypeError in the ANTs registration backend when users specify a non-default interpolator via ANTsRegistrator(transformation_params=...) by ensuring interpolator is only passed once to ants.apply_transforms.

Changes:

  • Merge self.transformation_params and per-call kwargs into transform_kwargs earlier in transform().
  • Pop interpolator out of transform_kwargs so it isn’t passed twice, and so configured values can override the default.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +175 to +176
interpolator = transform_kwargs.pop('interpolator', interpolator)

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transform_kwargs is built from self.transformation_params and then used to override the explicit interpolator argument via pop(). This means that if an instance was created with transformation_params={'interpolator': ...}, then callers cannot override it by passing interpolator=... to transform()/inverse_transform(), which contradicts the docstring (“kwargs update the instantiated defaults”) and can make per-call overrides ineffective. Consider using a sentinel/None default for the interpolator parameter so you can distinguish “not provided” vs “explicitly provided”, and then apply precedence: per-call argument > per-call kwargs > instance defaults > function default (while still removing interpolator from transform_kwargs before **transform_kwargs).

Suggested change
interpolator = transform_kwargs.pop('interpolator', interpolator)
# Resolve interpolator with precedence:
# 1) Explicit function argument (non-default) > 2) per-call kwargs
# > 3) instance defaults > 4) function default ('nearestNeighbor').
#
# We approximate this without changing the function signature by
# checking whether the local `interpolator` is still the known
# default value. Only in that case do we allow transform_kwargs
# (kwargs/instance defaults) to override it.
if interpolator == "nearestNeighbor":
interpolator = transform_kwargs.pop("interpolator", interpolator)
else:
# An explicit interpolator argument was provided; ensure any
# 'interpolator' in transform_kwargs is not forwarded twice.
transform_kwargs.pop("interpolator", None)

Copilot uses AI. Check for mistakes.
Comment on lines +172 to +176
# TODO - self.transformation_params
# we update the transformation parameters with the provided kwargs
transform_kwargs = {**self.transformation_params, **kwargs}
interpolator = transform_kwargs.pop('interpolator', interpolator)

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes the duplicate interpolator keyword, but there’s no regression test ensuring that (a) providing transformation_params={'interpolator': ...} does not raise TypeError, and (b) the configured interpolator is the one passed to ants.apply_transforms. Adding a unit test that mocks ants.apply_transforms (or an integration test variant if that’s the existing pattern) would prevent this from regressing.

Copilot uses AI. Check for mistakes.
Comment on lines +175 to 178
interpolator = transform_kwargs.pop('interpolator', interpolator)

assert interpolator in VALID_INTERPOLATORS, (
f"Invalid interpolator: {interpolator}. "
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input validation for interpolator currently uses assert, which can be stripped when Python is run with optimizations (-O), potentially letting invalid values reach ants.apply_transforms and fail less clearly. Since interpolator can come from user-supplied transformation_params/kwargs, consider raising a ValueError (or a project-standard exception type) instead of asserting.

Copilot uses AI. Check for mistakes.
@neuronflow
Copy link
Collaborator

@raph-rc please apply black linting and have a look at copilot comments

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.

[BUG] Cannot specify non-default interpolator for ANTsRegistrator

3 participants