Certain Transformations disregard *args and **kwargs (including copy)

nicoloesch commented 1 month ago

Is there an existing issue for this?

[X] I have searched the existing issues

Bug summary

Certain children of the base class torchio.Transform do not adhere to the *args and **kwargs provided during __init__ as they utilise already existing transformations in apply_transform. This mainly includes the kwarg copy, which is not passed to the transform utilised in apply_transform. However, other attributes may be required depending on the circumstances.

Examples of this are:

torchio.transforms.preprocessing.spatial.crop_or_pad.CropOrPad: L.282 and L.286 -> no instantiation of Crop/Pad with self.copy
torchio.transforms.preprocessing.label.sequential_labels.SequentialLabels: L. 55 -> no instantiation of RemapLabels with self.copy
torchio.transforms.preprocessing.spatial.ensure_shape_multiple.EnsureShapeMultiple: L.136 -> no instantiation of CropOrPad with self.copy
torchio.transforms.preprocessing.spatial.resize.Resize: L. 75 -> no instantiation of CropOrPad with self.copy

This extends to any transformation that re-utilises an already defined base transformation.

Code for reproduction

from torchio import CropOrPad
from torchio.datasets import Colin27

subject = Colin27()
transform = CropOrPad((64,64,64), copy=False)

transformed = transform(subject)

Actual outcome

The underlying transform in apply_transform is not called with copy=False as intended in the Constructor. This results in the copying of the respective copying of the Subject despite not defined by the user.

Error messages

No response

Expected outcome

All *args and **kwargs of any Transform should be passed onto each temporary/placeholder transform during apply_transform if it utilises an already implemented Transform opposed to a standalone implementation.

System info

Platform:   Linux-6.8.0-45-generic-x86_64-with-glibc2.35
TorchIO:    0.20.1
PyTorch:    2.3.1
SimpleITK:  2.4.0 (ITK 5.4)
NumPy:      2.0.2
Python:     3.10.15 (main, Oct  3 2024, 07:27:34) [GCC 11.2.0]

fepegar commented 1 month ago

Hi @nicoloesch. Can you please explain why this is important? I'm not sure I understand the practical implications, and your snippet seems to work as expected.

nicoloesch commented 1 month ago

Hi @fepegar,

The main argument of me filling this as a bug is the absence of copy arguments in the aforementioned transforms. As a result, the user has no control over the transform itself, as it uses a separate transform under the hood that does not adhere to the user-specified arguments of the outer transform.

E.g. if the user instantiates CropOrPad with any of the Transform base arguments that are passed using **kwargs, the transform utilises Crop/Pad under the hood that are instantiated without the user-specified **kwargs.

The reason I filled this bug is the relation to #1222 as we can instantiate CropOrPad with copy=False but the underlying transformation will use the default copy=True. As a result, we can avert one copy of the Subject but still have a second one floating around due to the "hidden" transformation in apply_transform.

As mentioned before, it extends beyond the copy argument and includes any of the the baseclass (Transform) kwargs including p, include, exclude, etc.

A better way to visualise the potentially faulty outcome would be to utilise exclude=.... I am aware that preprocessing transforms should be applied to all of the images but it is easier to see compared to profiling memory. (Maybe it might crash as the images are no longer of the same size after the transform if one excludes some ...).

romainVala commented 1 month ago

Yes I agree and the problem is quite general, (and important) because random transform always call non random transform at the end, without passing the copy argument I check at least for RandomAffine which cal Affine see https://github.com/fepegar/torchio/pull/1227#issuecomment-2437028606

(I should have put the comment here instead)

Note that when RandomAffine construct the Affine transform, the include / exclude argument are passed ...

For the proba p, I would say this is the only exception : We do not want to pass it through because it has already been taken into account. (So if RandomAffine pass it to Affine, then we would have a probability of p*p instead of p ...)

nicoloesch commented 3 weeks ago

Hi @romainVala,

Thanks for also having a closer look! I totally agree with your comment and will change the stacking or probabilities such that we are not having a p*p probability for transforms that re-utilise already defined transforms (such as all Random... transforms). I am also open to any other way to pass/store the arguments. I just thought a dictionary might be the easiest solution so we can pass it as **kwargs ...

fepegar / torchio