Refactored and modified private api for resize functional op #6191
Conversation
| @@ -412,19 +410,15 @@ def test_resize_scripted(self, dt, size, max_size, interpolation, device): | |||
| # This is a trivial cast to float of uint8 data to test all cases | |||
| tensor = tensor.to(dt) | |||
| if max_size is not None and len(size) != 1: | |||
| pytest.xfail("with max_size, size must be a sequence with 2 elements") | |||
| pytest.skip("Size should be an int or a sequence of length 1 if max_size is specified") | |||
There was a problem hiding this comment.
Question: Why skip instead of xfail?
There was a problem hiding this comment.
That's a good question :)
According to https://docs.pytest.org/en/stable/how-to/skipping.html
We skip in case windows tests on non-windows platform and xfail if we expect a test to fail for some reason.
In our case, the configuration "max_size is not None and len(size) != 1" can not be tested as we explicitly raise and error. Proper solution to that is to catch the error and check the message text with pytest.raises.
I felt like skipping is more appropriate than xfail. But it is a matter of taste if you think it is better to revert to xfail I can do that and will only fix the message which is incorrect
There was a problem hiding this comment.
I was expecting a pytest.raises where if I'm honest but no strong opinions.
…#6191) Summary: * Refactored and modified private api for resize functional op * Fixed failures * More updates * Fixed flake8 Reviewed By: NicolasHug Differential Revision: D37450358 fbshipit-source-id: 173d75ecb293eb418e9e7df9ff9eb152dd17f4ac
|
@vfdev-5 pointed me to this PR from our discussions in #6209 (comment) As discussed over there, |
It's possible if we introduce additional |
|
I guess we could just define it in |
|
@NicolasHug as far as I know those APIs are private and this change has been done many versions ago. I don't think we should worry about BC here. |
I'm not sure I understand. Wasn't this PR merged 5 days ago?
Unfortunately there is absolutely nothing that indicates that they are private. |
|
Sorry I was referring to when the API became private not to this PR. Let's continue our discussion at #6209 (comment) to avoid further confusion |
Description:
_compute_output_sizeand removedmax_sizearg fromF_t.resizeandF_pil.resize._compute_output_sizein proto for bboxes (right nowmax_sizeis not taken into account)resize_image_*. This will be fixed in [proto] Improvements for functional API and tests #6187