Simplify and organize test_ops. #1551
Conversation
pedrofreire
changed the title
There was a problem hiding this comment.
This looks much better, thanks a lot for the improvements @pedrofreire !
I only have minor comments which are not merge-blocking, and which can be fixed in a follow-up PR.
Tests seem to be failing on TravisCI though due to OOM on the gradcheck.
While we will be moving away from TravisCI soonish, we still rely on it for some signal, so it would be good to have it fixed.
Can you maybe reduce the input size for gradient computation to fix this OOM?
|
Link for failed tests on TravisCI: https://travis-ci.org/pytorch/vision/builds/607174087?utm_source=github_status&utm_medium=notification |
Codecov Report
@@ Coverage Diff @@
## master #1551 +/- ##
=======================================
Coverage 65.48% 65.48%
=======================================
Files 90 90
Lines 7080 7080
Branches 1077 1077
=======================================
Hits 4636 4636
Misses 2135 2135
Partials 309 309Continue to review full report at Codecov.
|
|
@pedrofreire this PR uncovered a small inconsistency with |
|
@pedrofreire #1556 has been merged, so if you rebase your PR on top of master all tests should pass. |
We perform the following: - Simplify the functions slow_roi_pooling, slow_ps_roi_pooling, slow_ps_roi_align and bilinear_interpolate (including finding and removing a semi-bug in slow_ps_roi_pooling, which used bin_w instead of bin_h); - Wrote a slow_roi_align function, that was missing; - Create a base class testing all combinations of forward/backward, cpu/cuda, contiguous/non-contiguous; - Organize all testing inside the base class with _test_forward and _test_backward (which can be easily overriden if a parciular op needs something different); an Op class then only needs to implement fn, get_script_fn, and expected_fn. A few points: - We are using the same inputs for all tests, and not trying all possible inputs in the domain of a given operation. One improvement would be to test more diverse inputs, and to personalize the inputs for some ops (e.g. different inputs for pooling ops and align ops). - Running all tests is quite slow (~1 min only for CPU tests), so that can possibly be improved.
gradcheck can be quite costly, and it was causing OOM errors and making the tests slow. By reducing the size of the input, the test speed is down to 3 seconds for the CPU tests. Other points: - We remove an unused namedtuple; - We inherit from object for better Python 2 compatibility; - We remove a hardcoded pool_size from the TorchScript functions, and add it as a parameter instead.
This should fix lint errors.
pedrofreire
force-pushed
the
imptestops
branch
from
7ccfb70 to
765b891
Compare
For #1528, We perform the following:
A few points: