Skip to content

Fix RoiAlign aligned=True export #4692

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 3 commits into from
Oct 26, 2021
Merged

Conversation

jiafatom
Copy link
Contributor

@jiafatom jiafatom commented Oct 21, 2021

The original code on aligned=True is not a complete solution and causes result mismatch.
The correct way is to have ONNX RoiAlign updated, that we have done recently for RoiAlign-16.
Therefore we have a fix here to handle aligned=True case.

cc @neginraoof

@jiafatom
Copy link
Contributor Author

cc @fmassa @datumbox Could you please sign off? Thanks.

@jiafatom
Copy link
Contributor Author

jiafatom commented Oct 25, 2021

friendly ping @fmassa @datumbox @pmeier @NicolasHug, thanks

Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

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

Thanks @jiafatom for the PR!

Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

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

Thanks @jiafatom, looks good to me. You may need to resolve the merge conflicts, before we can merge this.

@jiafatom
Copy link
Contributor Author

Thanks @jiafatom, looks good to me. You may need to resolve the merge conflicts, before we can merge this.

I am a little confused here, the conflicting files are just my changes, why it shows to conflicting files? Or what is the best way to do here?

@prabhat00155
Copy link
Contributor

Thanks @jiafatom, looks good to me. You may need to resolve the merge conflicts, before we can merge this.

I am a little confused here, the conflicting files are just my changes, why it shows to conflicting files? Or what is the best way to do here?

You don't have the latest changes in your branch. Check this:

batch_indices = _cast_Long(
.
Just fetch and merge the latest main, resolve the merge conflicts and update your PR. I can update this for you, if you are having issues with this.

@jiafatom
Copy link
Contributor Author

Thanks @jiafatom, looks good to me. You may need to resolve the merge conflicts, before we can merge this.

I am a little confused here, the conflicting files are just my changes, why it shows to conflicting files? Or what is the best way to do here?

You don't have the latest changes in your branch. Check this:

batch_indices = _cast_Long(

.
Just fetch and merge the latest main, resolve the merge conflicts and update your PR. I can update this for you, if you are having issues with this.

Thanks, done :)

@prabhat00155
Copy link
Contributor

@jiafatom You are getting some linter errors. You may need to run the code formatter locally and update the PR. Look at this https://github.com/pytorch/vision/blob/main/CONTRIBUTING.md#formatting.

@prabhat00155 prabhat00155 merged commit 175716b into pytorch:main Oct 26, 2021
@github-actions
Copy link

Hey @prabhat00155!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

@jiafatom jiafatom deleted the roi_true branch October 26, 2021 16:28
facebook-github-bot pushed a commit that referenced this pull request Oct 27, 2021
Summary: Co-authored-by: Prabhat Roy <[email protected]>

Reviewed By: NicolasHug

Differential Revision: D31957853

fbshipit-source-id: 5054da13eb08732c0702cc684a9b065b70fd2917
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
@pmeier pmeier mentioned this pull request Nov 30, 2021
This was referenced Nov 30, 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.

4 participants