Port to_tensor tests in test_transforms to pytest #3966
Conversation
|
Hi @DevPranjal! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
There was a problem hiding this comment.
Thanks @DevPranjal ! the changes look good to me, with some simple suggestions
test/test_transforms.py
Outdated
| with pytest.raises(TypeError): | ||
| trans(np.random.rand(1, height, width).tolist()) | ||
| trans(np.random.rand(1, height, width)) |
There was a problem hiding this comment.
I think we should have 2 raises blocks here, one per line. This is because we want to make sure that both lines below raise a TypeError. With a single block, we're only asserting that one of them raises the error. In fact I think the original test code was incorrect.
Also, we don't need those assertions to be part of the parametrization, it's a bit overkill as they don't need the channel parameter. So I'd suggest to extract them into a separate test_pil_to_tensor_errors() test function
test/test_transforms.py
Outdated
| with pytest.raises(TypeError): | ||
| trans(np.random.rand(1, height, width).tolist()) | ||
|
|
||
| with pytest.raises(ValueError): | ||
| trans(np.random.rand(height)) | ||
| trans(np.random.rand(1, 1, height, width)) |
There was a problem hiding this comment.
same here about having 1 raises block per line, and we should also move these out in a separate test function
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
…l/vision into refactor-test-ToTensor
Reviewed By: NicolasHug Differential Revision: D29027351 fbshipit-source-id: 23fd0b586b06e4448b6ca90577ecd3b7cb9af7fb
Refactor Group F as mentioned in #3945