RandomAffine raises an error when isotropic=True and 3 elements are given for scales

AminAlam commented 8 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Bug summary

In transforms.augmentation.spatial.random_affine, if the isotropic argument is given as True and 3 values are given as scales, _parse_scales_isotropic function will raise an error

Code for reproduction

spatial_transforms = {tio.RandomAffine(scales=(0.3, 0.3, 0.3), label_keys=['soma', 'trace'], isotropic=True, degrees=0, center='image', image_interpolation='bspline', label_interpolation='label_gaussian'): 0.4}

Actual outcome

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "./venv/lib/python3.11/site-packages/torchio/transforms/augmentation/spatial/random_affine.py", line 127, in __init__
    _parse_scales_isotropic(scales, isotropic)
  File ./venv/lib/python3.11/site-packages/torchio/transforms/augmentation/spatial/random_affine.py", line 447, in _parse_scales_isotropic
    raise ValueError(message)
ValueError: If "isotropic" is True, the value for "scales" must have length 1 or 2, but "(0.3, 0.3, 0.3)" was passed

Error messages

If "isotropic" is True, the value for "scales" must have length 1 or 2, but "3" was passed

Expected outcome

No error should be raised as in the isotropic mode the only condition should be met is s1==s2==s3 :)

System info

Platform:   Linux-6.1.0-17-amd64-x86_64-with-glibc2.36
TorchIO:    0.19.6
PyTorch:    2.2.1+cu121
SimpleITK:  2.3.1 (ITK 5.3)
NumPy:      1.26.4
Python:     3.11.2 (main, Mar 13 2023, 12:18:29) [GCC 12.2.0]

arsalanfiroozi commented 7 months ago

I have the same issue.

romainVala commented 7 months ago

Hello I aggree it seems a bug to fix, but the work-around seems quite easy either set the scale with 3 number, but put isotropic to False or just set cale with 1 number (since you want it to be isotropic). In this case you do not even need to set the "isotropic" argument (default False will also be fine). (should work exactly the same with issotropic set to True)

May be I miss something, (@fepegar ?) but my understanding is that "isotropic" argument is not usefull

AminAlam commented 7 months ago

Hi and thanks for your response. I agree that there are workarounds for this bug, but I believe it's better to fix it. I fixed it in this pull request using a single line of code #1159

fepegar commented 7 months ago

I think the code logic is correct, but maybe the docs and the error message are not clear. The idea is that a single scale value for all dimensions is sampled if isotropic is True. The number of scales must be one or two because if it's one (let's call it $s$, the min and max along all dimensions will be $[1-s, 1+s]$. If it's two, the values will be the min and max scale. If it's three, it's the former case for each dimension, and six means the latter for each dimension.

Does that make sense?

romainVala commented 7 months ago

hmm, the logic I do not understand is when is the isotropic argument necessary ? My understanding is that isotropic is implicit when you specify parameter of length 1 or 2

taff1 = tio.RandomAffine(scales=0.2)
taff2 = tio.RandomAffine(scales=(0.8, 1.5))

print(f'Scale is {taff1.scales}   isotropic is {taff1.isotropic} ')
print(f'Scale is {taff2.scales}   isotropic is {taff2.isotropic} ')

Scale is (0.8, 1.2, 0.8, 1.2, 0.8, 1.2)   isotropic is False 
Scale is (0.8, 1.5, 0.8, 1.5, 0.8, 1.5)   isotropic is False

AminAlam commented 7 months ago

Thanks @fepegar, it makes sense to me now. I have modified the error message and the documentation to make it more clear. Please find them in this #1163.

@romainVala Based on this code in spatial.radom_affine.RandomAffien.get_params:

scaling_params = self.sample_uniform_sextet(scales)
  if isotropic:
      scaling_params.fill_(scaling_params[0])

If "isotropic" is True, then the scaling factor of all other dimensions is filled with the first scaling factor, so when "scales" only has 1 or 2 values and "isotropic" is set as True, we make sure that all dimensions will be scaled with the same factor. In your example, the values of scales are lower and upper bounds of random sampling, not the actual values of scaling in the transformation.

romainVala commented 7 months ago

not sure to understand properly ... scale input parameter for RandomAffine is to specify the lower and upper bound ... in order to do a random selection. If you want to force RandomAffine to get a specific value (for instance 0.8), then you need to restrict the bound to (0.8, 0.8) ... or may be, since you do not want randomness, then directly go with the Affine transform

I agree with the code snipet you show, but my example demonstrate that this line of code is not needed.

AminAlam commented 7 months ago

@romainVala Imagine that we have set the bounds as (1,3), then the scales will be randomly selected as (1.3, 1.5, 2.8). If "isotropic" is True, then we force the scales to be (1.3, 1.3, 1.3). The scaling factor is chosen randomly but as isotropic is True, it will be same for all dimensions.

fepegar commented 7 months ago

@AminAlam, that's the intended behavior indeed.

romainVala commented 7 months ago

Ok, my bad I was too focus on the extension of input parameter, But you are right isotropic is for enforcing the chosen random values to be isotropic sorry for my confusions

fepegar / torchio