Skip to content

Simplify / improve test_ops #1528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fmassa opened this issue Oct 28, 2019 · 3 comments
Closed

Simplify / improve test_ops #1528

fmassa opened this issue Oct 28, 2019 · 3 comments

Comments

@fmassa
Copy link
Member

fmassa commented Oct 28, 2019

test_ops.py implements tests for the C++ operators in torchvision.
Given that torchvision.ops contain dedicated CPU / CUDA implementations, they need to test for correctness on both CPU and GPU, comparing that the outputs are the same. Plus, they also implement backward computation for most ops (except NMS, which is fine as is), which means that we also need to test the gradients.

In its current form, most tests in test_ops.py have a lot of duplicate code between each test, with generally just a few lines changing between the tests. We should see if there are things we can factorize to reduce code duplication and improve maintanability.
We should also stop hard-coding values for the gradients / outputs, and we should instead rely on gradcheck to perform this checking.
Plus, TorchScript support is now handled as an extra check for each op

vision/test/test_ops.py

Lines 191 to 195 in f612182

@torch.jit.script
def script_func(input, rois):
return ops.roi_pool(input, rois, 5, 1.0)[0]
self.assertTrue(gradcheck(lambda x: script_func(x, rois), (x,)), 'gradcheck failed for scripted roi_pool')
but more consistency could be better.

Ideally, we would have a simpler set of tests, which still has the same coverage for contiguous / non-contiguous / CPU / CUDA / forward / backward, which has less redundancy.
How to approach this? We could have a base test class that defines all we want to test, and each operator test inherits from it.

cc @pedrofreire for visibility

@pedrofreire
Copy link
Contributor

I'd be happy to have a look on this :)

@fmassa
Copy link
Member Author

fmassa commented Oct 28, 2019

@pedrofreire go for it!

@fmassa
Copy link
Member Author

fmassa commented Nov 6, 2019

Fixed via #1551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants