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

Added rois shape check in C++ #2794

Merged
merged 5 commits into from
Oct 13, 2020

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Oct 12, 2020

Related to #2779

Desription:

  • Added torch check of rois 1st dimension for all ROI* ops
  • Replaced old AT_ASSERT/ERROR by new TORCH_CHECK
  • No C++ tests added

@vfdev-5 vfdev-5 force-pushed the add-cpp-check-rois-shape branch from 6422016 to 015c170 Compare October 12, 2020 21:12
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #2794 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2794   +/-   ##
=======================================
  Coverage   73.22%   73.22%           
=======================================
  Files          96       96           
  Lines        8446     8446           
  Branches     1320     1320           
=======================================
  Hits         6185     6185           
  Misses       1859     1859           
  Partials      402      402           
Impacted Files Coverage Δ
torchvision/ops/_utils.py 82.60% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42e7f1f...9925ab9. Read the comment docs.

Copy link
Member

@fmassa fmassa 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!

Unfortunately the location of the checks is not correct, and they should be pushed up to the cpu / cuda functions.

This is because the dispatcher (which currently is only setup for NMS and RoIAlign) don't perform argchecking in the main dispatcher code, and thus those checks are probably going to be skipped.
See the PR porting NMS to use the dispatcher in 4e8deeb to see that we had subtle errors and had to duplicate the checks in the cpu / cuda folders.

Taking a comment from @ezyang who pointed out the issue

but right now, you have to make a helper function and make sure the cpu and cuda functions call that helper function manually

do NOT put size checks in the dispatching function, you are not guaranteed to call that function, and also if you put the checks there you'll do them repeatedly uselessly

- Replaced old AT_ASSERT/ERROR by new TORCH_CHECK
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@fmassa fmassa merged commit b8e9308 into pytorch:master Oct 13, 2020
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* Added rois shape check in C++

* Fixes code formatting

* Remove accidental include

* - Updated code according to the review
- Replaced old AT_ASSERT/ERROR by new TORCH_CHECK
vfdev-5 added a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* Added rois shape check in C++

* Fixes code formatting

* Remove accidental include

* - Updated code according to the review
- Replaced old AT_ASSERT/ERROR by new TORCH_CHECK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants