Skip to content

[ONNX] Fix roi_align ONNX export #3355

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

Merged
merged 13 commits into from
Mar 12, 2021
Merged

Conversation

KsenijaS
Copy link
Contributor

@KsenijaS KsenijaS commented Feb 5, 2021

Allow export of roi_align when sampling rate is negative

@KsenijaS KsenijaS changed the title [ONNX] Fix roi_align ONNX export [WIP][ONNX] Fix roi_align ONNX export Feb 5, 2021
@fmassa
Copy link
Member

fmassa commented Mar 2, 2021

@KsenijaS is the PR ready for review?

@KsenijaS KsenijaS changed the title [WIP][ONNX] Fix roi_align ONNX export [ONNX] Fix roi_align ONNX export Mar 4, 2021
@KsenijaS
Copy link
Contributor Author

KsenijaS commented Mar 4, 2021

@KsenijaS is the PR ready for review?

@fmassa Yes, PR is ready for a review. It would be also good that some one from my team review it as well.
@neginraoof @BowenBao

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.

The changes look generally good to me, but there is a syntax error in the warning message that needs to be fixed before merging

@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #3355 (8c7045c) into master (9a08e47) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3355      +/-   ##
==========================================
- Coverage   78.86%   78.83%   -0.03%     
==========================================
  Files         105      105              
  Lines        9752     9753       +1     
  Branches     1567     1568       +1     
==========================================
- Hits         7691     7689       -2     
- Misses       1574     1577       +3     
  Partials      487      487              
Impacted Files Coverage Δ
torchvision/ops/_register_onnx_ops.py 48.57% <0.00%> (-4.56%) ⬇️
torchvision/datasets/mnist.py 55.21% <0.00%> (-0.39%) ⬇️

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 0e80296...e8d64cc. Read the comment docs.

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, thanks!

@datumbox datumbox merged commit 11268ca into pytorch:master Mar 12, 2021
facebook-github-bot pushed a commit that referenced this pull request Mar 18, 2021
Summary:
* add tests

* fix bug

* remove tests

* fix comment

* fix comment

* add warning

* fix syntax error

* fix python lint

Reviewed By: fmassa

Differential Revision: D27127999

fbshipit-source-id: c416f4de87a50a6d5fe3d006341fb84500f2a20d

Co-authored-by: Vasilis Vryniotis <[email protected]>
@datumbox datumbox added bug and removed fix labels Apr 27, 2021
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.

5 participants