Test error in test_soft_clipping_one_sided_high #7616

Activity

1. KumoLiu commented on Apr 10, 2024

   

   KumoLiu

   on Apr 10, 2024

   ContributorAuthor

   More actions

   cc @Lucas-rbnt
2. Lucas-rbnt commented on Apr 10, 2024

   

   Lucas-rbnt

   on Apr 10, 2024

   Last edited by Lucas-rbnt

   Contributor

   More actions

   Hello, I think this is because torch.logaddexp and numpy.logaddexp give different results.

   One workaround would be, when the input is a numpy array, to convert it to tensor to apply torch.logaddexp and then convert it back to numpy. Not very elegant but simple and should solve the problem

   A small test on this yields convincing results:

   from monai.transforms.utils_pytorch_numpy_unification import softplus
   
   x = torch.randn(128, 128)
   np.array_equal(softplus(x.numpy()), softplus(x).numpy())
   :> False

   while the modified version works as expected:

   import numpy as np
   import torch
   
   def softplus(x):
       """stable softplus through `np.logaddexp` with equivalent implementation for torch.
       Args:
           x: array/tensor.
       Returns:
           Softplus of the input.
       """
       if isinstance(x, np.ndarray):
           x = torch.from_numpy(x)
           return torch.logaddexp(torch.zeros_like(x), x).numpy()
       return torch.logaddexp(torch.zeros_like(x), x)
   
   x = torch.randn(128, 128)
   np.array_equal(softplus(x.numpy()), softplus(x).numpy())
   :> True

   Let me know if you think this is relevant and sorry about that!
3. KumoLiu commented on Apr 10, 2024

   

   KumoLiu

   on Apr 10, 2024

   ContributorAuthor

   More actions

   Hi @Lucas-rbnt, I believe we could consider adjusting this line

   MONAI/tests/test_clip_intensity_percentiles.py

   Line 69 in e9a5bfe

   expected = soft_clip(self.imt, sharpness_factor=1.0, minv=None, maxv=upper)

   to:

   expected = soft_clip(im, sharpness_factor=1.0, minv=None, maxv=upper, dtype=im.dtype)
   

   I propose that we use im in place of self.imt and then set dtype. This approach would potentially circumvent the issue tied to the situation where results are consistently checked against NumPy version. I'm also of the thought that this could prove beneficial for other test cases included within the test file. Do you think this would be a viable solution?
   Also, would you mind creating a PR to fix this one, thanks!
4. Lucas-rbnt commented on Apr 11, 2024

   

   Lucas-rbnt

   on Apr 11, 2024

   Contributor

   More actions

   It's definitely a way of doing things, but it won't be enough on its own because in the __call__ of the ClipIntensityPercentiles class x is converted into a tensor, as in many other transforms:

       def __call__(self, img: NdarrayOrTensor) -> NdarrayOrTensor:
           """
           Apply the transform to `img`.
           """
           img = convert_to_tensor(img, track_meta=get_track_meta())

   Then the softplus is applied directly to a tensor and not a numpy array, and we get back to the differences in results between np.logaddexp and torch.logaddexp. So this should also be changed.
   Both are possible, but I wonder if it wouldn't be better to make a choice in order to have consistent results between numpy and torch, so that a tensor or an array containing the same values has the same output through the softplus function.
5. KumoLiu

   added a commit that references this issue on Apr 11, 2024

   fix Project-MONAI#7616

   ...

   This issue will close once commit 7a25c3a is merged into the 'dev' branch.

   7a25c3a
6. KumoLiu commented on Apr 11, 2024

   

   KumoLiu

   on Apr 11, 2024

   ContributorAuthor

   More actions

   Hi @Lucas-rbnt,

   Absolutely, I understand your perspective. Since the introduction of MetaTensor in version 0.9.1, our transformations have consistently been converting data into the MetaTensor format.
   More importantly, there isn't an overarching requirement for rendering identical outputs from numpy and PyTorch, particularly for logaddexp. PyTorch has its own internal safeguards to ensure that the results don't stray substantially from numpy outputs.
   https://github.com/pytorch/pytorch/blob/793df52dc52f5f5f657744abfd7681eaba7a21f9/test/test_binary_ufuncs.py#L3461
   In the grand scheme of things, it doesn't seem imperative for users to cross-verify the consistency of results between torch and numpy.

   What do you think?
   Thanks
7. Lucas-rbnt commented on Apr 11, 2024

   

   Lucas-rbnt

   on Apr 11, 2024

   Contributor

   More actions

   Yes, I agree, it remains coherent from my point of view!
   However, I'm not sure that the PR will fix entirely the problem since there is still the MetaTensor conversion.
   To ensure that the tests are successful, perhaps we should use the same tolerance to compare the tables as in the torch tests you mentioned?

   self.assertEqual(ref, v.float(), atol=0.01, rtol=0.01)

   (l.3461) with torch.bfloat16

   and it seems to me that https://github.com/pytorch/pytorch/blob/793df52dc52f5f5f657744abfd7681eaba7a21f9/torch/testing/_comparison.py#L1183 gives the others default parameter depending on the type

    +---------------------------+------------+----------+
       | ``dtype``                 | ``rtol``   | ``atol`` |
       +===========================+============+==========+
       | :attr:`~torch.float16`    | ``1e-3``   | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.bfloat16`   | ``1.6e-2`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.float32`    | ``1.3e-6`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.float64`    | ``1e-7``   | ``1e-7`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.complex32`  | ``1e-3``   | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.complex64`  | ``1.3e-6`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.complex128` | ``1e-7``   | ``1e-7`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.quint8`     | ``1.3e-6`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.quint2x4`   | ``1.3e-6`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.quint4x2`   | ``1.3e-6`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.qint8`      | ``1.3e-6`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | :attr:`~torch.qint32`     | ``1.3e-6`` | ``1e-5`` |
       +---------------------------+------------+----------+
       | other                     | ``0.0``    | ``0.0``  |
       +---------------------------+------------+----------+
   

   https://github.com/pytorch/pytorch/blob/793df52dc52f5f5f657744abfd7681eaba7a21f9/torch/testing/_comparison.py#L1183

   However, the values for float32 are quite low compared to the error mentioned above, so I'm not sure.
8. KumoLiu commented on Apr 11, 2024

   

   KumoLiu

   on Apr 11, 2024

   ContributorAuthor

   More actions

   Taking into account that PyTorch innately performs a comprehensive array of internal consistency tests to ensure alignment with NumPy, it could be redundant for us to independently verify this consistency within our codebase.

   Moreover, I would like to suggest that we prioritize merging this fix given that it has frequently led to test failures in other PRs:
   https://github.com/Project-MONAI/MONAI/actions/runs/8644324879/job/23699221155?pr=7625#step:8:12528
   #7604

   Let's move this discussion onward to garner more opinions on this matter. cc @ericspod @atbenmurray @Nic-Ma
9. KumoLiu

   closed this as completedin #7624on Apr 11, 2024
10. KumoLiu

    added a commit that references this issue on Apr 11, 2024

    Fix test error in `test_soft_clipping_one_sided_high` (#7624)

    ...

    This issue will close once commit 54a6991 is merged into the 'dev' branch.

    54a6991