Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Affine transform currently does not have an affine matrix parameter #3313

Closed
Spenhouet opened this issue Nov 11, 2021 · 8 comments · Fixed by #3318 or #3326
Closed

The Affine transform currently does not have an affine matrix parameter #3313

Spenhouet opened this issue Nov 11, 2021 · 8 comments · Fixed by #3318 or #3326
Assignees

Comments

@Spenhouet
Copy link
Contributor

Is your feature request related to a problem? Please describe.
The Affine transform forwards its parameters to the AffineGrid class.
But it currently does not provide the "affine" parameter which the AffineGrid class also has.

This allows to directly define a affine transformation matrix instead of individual parameters for transformations like rotation.
If computations are directly done with affine transformation matrices, this parameter is really helpful (necessary).

Describe the solution you'd like
The only change necessary, is to add the affine parameter and to forward it to the AffineGrid class.

class Affine(Transform):
    ...
    def __init__(
        ...
        affine: Optional[NdarrayOrTensor] = None,
        ...
    ) -> None:
        ...
        self.affine_grid = AffineGrid(
            ...
            affine=affine,
            ...
        )

Describe alternatives you've considered
We patched the affine transform locally.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 11, 2021

Hi @Spenhouet ,

Thanks for your feedback.
@wyli @ericspod I think this feature request can help improve the user experience, if you guys don't have other concerns, I will try to add it based on the suggestion here.

Thanks in advance.

@Nic-Ma Nic-Ma self-assigned this Nov 11, 2021
@wyli
Copy link
Contributor

wyli commented Nov 11, 2021

Sure I feel it might be simpler to have a warp function instead of transform class (unless we cache the grid for performance)

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 11, 2021

Hi @wyli ,

I think maybe we can just add an arg affine to Affine transform:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/spatial/array.py#L1432
If affine is not None, ignore the rotate_params, shear_params, translate_params, scale_params args.
That would be a non-breaking change, what do you think?

Thanks.

@wyli
Copy link
Contributor

wyli commented Nov 11, 2021

I don't know what's the benefit here, there's not much benefit compared with "affine.resampler(img, affine.affinegrid(affine)[0])" unless we have additional caching capability

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Nov 11, 2021

Hi @wyli ,

I think maybe @Spenhouet 's request is to set a fixed affine matrix in the transform chain, so adding an arg is easier to use?

Thanks.

@Spenhouet
Copy link
Contributor Author

Spenhouet commented Nov 11, 2021

Yes, I also do not see the harm of adding this parameter. @wyli or what is your worry?

It would also enable to easily perform the inverse operation in the chain.

We have a transform which currently contains these lines:

projection = np.linalg.inv(target_affine) @ affine
transform = Affine(affine=projection[:3, :3], mode=self.mode)

transformed_matrix: torch.Tensor
transformed_matrix, _ = transform(img.squeeze(0)) 

Sure, we could also do this:

projection = np.linalg.inv(target_affine) @ affine
transform = Affine(mode=self.mode)

transformed_matrix: torch.Tensor
transformed_matrix = transform.resampler(img.squeeze(0), transform.affinegrid(projection[:3, :3])[0])

But compared to simply adding the additional parameter, I don't think this is a great way to solve this.

@wyli
Copy link
Contributor

wyli commented Nov 11, 2021

We don't normally compose the array transforms. also, if we follow this pattern we'll build complicated transforms with many input arguments and if-else branches, then we lose the readability.

@Spenhouet
Copy link
Contributor Author

Spenhouet commented Nov 11, 2021

@wyli Maybe I misunderstood. We are not using the method to apply a static Affine in the pipeline.
We are in fact using it like mentioned above.

I get where you are coming from, but I wonder if this is the right case to focus on this.
The if-else branches already exist. The suggestion simply exposes the affine parameter to the Affine transform to not need to implement it like you suggested.

I don't think it will make the Affine transform any more complicated.

EDIT: But it will make other methods which use this Affine transform simpler. So we save on complexity there.

EDIT2: Or how would you rate the code complexity on these two code snippets:

transform = Affine(affine=projection[:3, :3], mode=self.mode)
transformed_matrix, _ = transform(img.squeeze(0)) 

vs.

transform = Affine(mode=self.mode)
transformed_matrix = transform.resampler(img.squeeze(0), transform.affinegrid(projection[:3, :3])[0])

EDIT3: Also the first one will also output the resulting affine while the second will not and will need additional code (further increasing code complexity and boilerplate).

Spenhouet pushed a commit to Spenhouet/MONAI that referenced this issue Nov 11, 2021
Signed-off-by: Sebastian Penhouet <s.penhouet@outlook.de>
Spenhouet pushed a commit to Spenhouet/MONAI that referenced this issue Nov 11, 2021
Signed-off-by: Sebastian Penhouet <sebastian.penhouet@airamed.de>
Nic-Ma added a commit that referenced this issue Nov 12, 2021
Signed-off-by: Sebastian Penhouet <sebastian.penhouet@airamed.de>

Co-authored-by: Sebastian Penhouet <sebastian.penhouet@airamed.de>
Co-authored-by: Nic Ma <nma@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants