Port test_ops.py to pytest #3953
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NicolasHug
commented
Jun 3, 2021
Comment on lines
+20
to
+22
| @pytest.mark.parametrize('device', cpu_and_gpu()) | ||
| @pytest.mark.parametrize('contiguous', (True, False)) | ||
| def test_forward(self, device, contiguous, x_dtype=None, rois_dtype=None, **kwargs): |
There was a problem hiding this comment.
The previously manual cross-product between device and contiguous is now seamlessly handled by the parametrization, both for test_forward and test_backward
pmeier
reviewed
Jun 3, 2021
There was a problem hiding this comment.
Looking good @NicolasHug. Two comments:
- Can we declare
RoIOpTesteranabc.ABCand mark the abstract methods as such? Makes it easier to read and catches errors faster if we need to add a new test case. - Can the
_test_boxes_shapemerged into the base class with the same pattern as the other tests?
|
Thanks for the review @pmeier ! Your comments were addressed :)
Yeah it was a bit weird. I just kept @cpu_only
def test_boxes_shape(self):
self._helper_boxes_shape(ops.roi_pool) |
pmeier
approved these changes
Jun 4, 2021
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 10, 2021
Reviewed By: NicolasHug Differential Revision: D29027309 fbshipit-source-id: 7704b4e86a7f476f405a981112b73677d0bff16c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ports the entire
test_ops.pyfile to pytest. It went pretty smoothly considering the size of the file, as the class structure already in place really helped.The diff looks a bit big but it's mainly because I parametrized some methods, so the indentation of the (now removed) for loops isn't here anymore.
The rest is just adding a bunch of decorators and changing class names so that they're properly discovered by pytest