Skip to content

Conversation

YosuaMichael
Copy link
Contributor

@YosuaMichael YosuaMichael commented Jun 27, 2022

Addressing #6207

(the bug is introduced on #6191 )

@YosuaMichael YosuaMichael requested a review from datumbox June 27, 2022 15:57
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM. Since our CI doesn't check for accimage (maybe it should, potential improvement for when we review our testing strategy), would you be able to offer proof that the test passes using accimage locally or on the dev server?

@YosuaMichael
Copy link
Contributor Author

YosuaMichael commented Jun 27, 2022

LGTM. Since our CI doesn't check for accimage (maybe it should, potential improvement for when we review our testing strategy), would you be able to offer proof that the test passes using accimage locally or on the dev server?

Yeah, I will take note about this for potential improvement on our testing CI.

I have tested it on dev server in AWS with accimage installed. Here is the test result:

For the test_accimage_resize only:

pytest test_transforms.py -k test_accimage_resize
=================================================================== test session starts ====================================================================
platform linux -- Python 3.9.12, pytest-7.1.2, pluggy-1.0.0
rootdir: /fsx/users/yosuamichael/repos/vision, configfile: pytest.ini
plugins: anyio-3.6.1, cov-3.0.0
collected 1313 items / 1312 deselected / 1 selected                                                                                                        

test_transforms.py .                                                                                                                                 [100%]

===================================================================== warnings summary =====================================================================
test/test_transforms.py::TestAccImage::test_accimage_resize
  /fsx/users/yosuamichael/repos/vision/torchvision/transforms/transforms.py:329: UserWarning: Argument 'interpolation' of type int is deprecated since 0.13 and will be removed in 0.15. Please use InterpolationMode enum.
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
====================================================== 1 passed, 1312 deselected, 1 warning in 1.16s =======================================================

For all transforms

$ pytest test_transforms.py 
=================================================================== test session starts ====================================================================
platform linux -- Python 3.9.12, pytest-7.1.2, pluggy-1.0.0
rootdir: /fsx/users/yosuamichael/repos/vision, configfile: pytest.ini
plugins: anyio-3.6.1, cov-3.0.0
collected 1313 items                                                                                                                                       

test_transforms.py ................................................................................................................................. [  9%]
.................................................................................................................................................... [ 21%]
.................................................................................................................................................... [ 32%]
....................................................................s..........................................................x.................... [ 43%]
.................................................................................................................................................... [ 54%]
.................................................................................................................................................... [ 66%]
.................................................................................................................................................... [ 77%]
.................................................................................................................................................... [ 88%]
.................................................................................................................................................... [100%]

===================================================================== warnings summary =====================================================================
test/test_transforms.py::TestAccImage::test_accimage_resize
  /fsx/users/yosuamichael/repos/vision/torchvision/transforms/transforms.py:329: UserWarning: Argument 'interpolation' of type int is deprecated since 0.13 and will be removed in 0.15. Please use InterpolationMode enum.
    warnings.warn(

test/test_transforms.py: 361 warnings
  /fsx/users/yosuamichael/repos/vision/test/test_transforms.py:2081: DeprecationWarning: `np.int` is a deprecated alias for the builtin `int`. To silence this warning, use `int` by itself. Doing this will not modify any behavior and is safe. When replacing `np.int`, you may wish to use e.g. `np.int64` or `np.int32` to specify the precision. If you wish to review your current use, check the release note link for additional information.
  Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
    res = np.floor(np.dot(inv_true_matrix, input_pt)).astype(np.int)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================= short test summary info ==================================================================
SKIPPED [1] test_transforms.py:934: Temporarily disabled
XFAIL test_transforms.py::test_max_value_iinfo
  torch.iinfo() is not supported by torchscript. See https://github.com/pytorch/pytorch/issues/41492.
=========================================== 1311 passed, 1 skipped, 1 xfailed, 362 warnings in 139.90s (0:02:19) ===========================================

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jun 27, 2022

@YosuaMichael thanks for fixing and sorry the bug ! In the PR introduced the type change it was tricky to please mypy and jit with Tuples

@YosuaMichael
Copy link
Contributor Author

YosuaMichael commented Jun 27, 2022

@YosuaMichael thanks for fixing and sorry the bug ! In the PR introduced the type change it was tricky to please mypy and jit with Tuples

@vfdev-5 just to confirm, this function don't need to be jit scriptable right? (I saw it has @torch.jit.unused decorator).
I have check with mypy, but not with jit for this.

Also no worry about it, this is quite tricky since the CI dont cover accimage and it only fail on accimage (luckily the internal test actually test on accimage and thats why I can catch it)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jun 27, 2022

@YosuaMichael right this function can't be jit scriptable but I was refactoring some common code into _compute_output_size which compute the output for this function and another one which is jit scriptable.
Maybe, there can be a solution to make everything working with tuples...

@YosuaMichael YosuaMichael merged commit f75272f into pytorch:main Jun 27, 2022
facebook-github-bot pushed a commit that referenced this pull request Jun 27, 2022
Reviewed By: datumbox

Differential Revision: D37460134

fbshipit-source-id: 921a3fa562f543f0a0fefbd6e57d1ea24781d26c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants