-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Added center as top-left for shear X/Y ops for autoaugment #5285
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
Conversation
💊 CI failures summary and remediationsAs of commit 6412fb0 (more details on the Dr. CI page):
3 failures not recognized by patterns:
This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vfdev-5 Thanks a lot for the investigations and for sending the PR.
We had long discussions with Victor about this because it's a tough call but I'll summarize here the reasons why we believe we should merge this BC-breaking change. After looking into the original implementations of AutoAugment and RandAugment, it became clear that their Shear method was using the top-left as the center. Unfortunately TorchVision didn't support this option until it was raised by the author of TrivialAugment @SamuelGabriel at #5204. This means that our implementation deviated from the canonical and thus we consider this discrepancy a bug.
Though I don't believe that this discrepancy had major effect on the overall accuracy of the models that we trained previously, I think it's worth aligning TorchVision's behaviour with the rest of the ecosystem as the majority of frameworks and libraries we checked all implement the Shear operator with center the top-left corner.
I recommend leaving this PR unmerged for a couple of days to allow for people to comment. If there are no objections we can merge it early next week.
Fixes #5204
Description:
@datumbox do you think we should add something else here to be fully consistent ?
Here is how affine with center=(0, 0) behaves for shear X and Y vs using PIL as in TF:
cc @SamuelGabriel
EDIT:
Links to TF implementation uses Pillow with shear parametrized such that fixed point is top-left:
However, official vision ops refers as well autoaugment paper and does shear with TF raw ops, the only difference is that they do fill_mode="reflect" by default:
Output for

magnitude = .4
shear X with TF from official vision ops: