Skip to content
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

Refactor BoxOps tests to use parameterize #5380

Merged
merged 7 commits into from
Feb 8, 2022
Merged

Conversation

HeungwooLee
Copy link
Contributor

@HeungwooLee HeungwooLee commented Feb 4, 2022

Closes #4500

Refactored TestBoxArea, TestBoxIoU and TestGenBoxIoU class to not have duplicated code.

  • Tests
    • formatting and typing: got warnings and errors but not from the change of this commit
    • pytest test -vvv: passed

@facebook-github-bot
Copy link

facebook-github-bot commented Feb 4, 2022

💊 CI failures summary and remediations

As of commit 619851f (more details on the Dr. CI page):


None of the CI failures appear to be your fault 💚



🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet.


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@HeungwooLee
Copy link
Contributor Author

1st commit was failed with lint failure Format files with µfmt
However, as there wasn't anything mentioned required this on CONTRIBUTING.md, I have go ahead and followed instruction from https://ufmt.omnilib.dev/en/stable/ but got more failures. Need some guidance to address failures.

@datumbox
Copy link
Contributor

datumbox commented Feb 4, 2022

@HeungwooLee Thanks! I will have a closer look next week.

Just not that the failing linter job is related to you PR. Have a look to this section of the guide on how to setup and use the same linter.

Don't worry about binary_win_wheel_py3.9_cu115 it's not broken due to your PR.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @HeungwooLee. I've added a few comments, let me know what you think.

test/test_ops.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
test/test_ops.py Outdated Show resolved Hide resolved
@HeungwooLee
Copy link
Contributor Author

HeungwooLee commented Feb 7, 2022

Just updated with 3rd commit which should address all comments above.

  • formatting: ufmt format test/test_ops.py
  • test: pytest test/test_ops.py -vvv: passed
  • type check: mypy --config-file mypy.ini: this failed with error not related to change torchvision/models/quantization/utils.py:50: error: Module has no attribute "fuse_modules_qat"; maybe "fuse_modules"?

Also updated with 4th commit for first two styling comments. ufmt is keep reverting this change for some reason and had to manually update them and skiped styling check.

@HeungwooLee
Copy link
Contributor Author

Addressed typing errors and updated PR with new commit.

  • formatting: ufmt format test/test_ops.p then manually reverted 2 changes with ** operator
  • typing: mypy --config-file mypy.ini test/test_ops.p
    • 13 [misc] errors complaining about methods used inside pytest.param(...) doesn't have at least self argument or being static class. As other examples in file were ignoring this error, I assume this is somewhat acceptable. Let me know if this is still an issue.
  • test: pytest test/test_ops.py -vvv (passed)
  • automated checks: I believe two failures are not related to my change.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only one final comment and we should be good:

test/test_ops.py Outdated Show resolved Hide resolved
@datumbox
Copy link
Contributor

datumbox commented Feb 8, 2022

@HeungwooLee Thanks for the awesome work, LGTM.

@datumbox datumbox merged commit 6032775 into pytorch:main Feb 8, 2022
facebook-github-bot pushed a commit that referenced this pull request Feb 11, 2022
Summary:
* Refactor BoxOps tests to use parameterize

* Refactor BoxOps tests to use parameterize

* Refactor BoxOps to use parameterize, addressed comments from PR#5380

* Refactor BoxOps to use parameterize, addressed minor styling comments from PR#5380

* Refactor BoxOps to use parameterize, addressed typing errorsfrom PR#5380

* Refactor BoxOps to use parameterize, addressed minor naming comments for PR#5380

Reviewed By: NicolasHug

Differential Revision: D34140248

fbshipit-source-id: 3844d49b78b3b8f4d432ef64094b43605d07351e
@pmeier pmeier mentioned this pull request Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor BoxOps tests to use parameterize
3 participants