-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Integration of TrivialAugment with the current AutoAugment Code #4221
Integration of TrivialAugment with the current AutoAugment Code #4221
Conversation
Hi @SamuelGabriel! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
I'd love some comment on this / review. For example by you, @datumbox? |
@SamuelGabriel Apologies for the delayed response. I was OOO. Now I'm back and trying to catch up. I'll get back to you as soon as possible. |
Thank You! No worries, I guess new features are not very urgent compared to other things. :) |
@SamuelGabriel They are important! I was just away for sometime and now try to catch up. This is on the top of my list so I will try to get back to you within the next week. BTW I see that there are a few conflicts with master. Do you prefer to resolve now or wait for me to review first? For full transparency I didn't have the chance to check out the PR yet. |
After fixing the diff and fixing the fixes, I have a mergeable version now. :) (Some wheels failed, but I believe this is due to something unrelated.) |
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.
@SamuelGabriel Thanks a lot for the contribution.
Your PR is very well-written and I like the proposed refactoring of the original AutoAugment algorithm. The only comment I left below is related to a convention we have for Transforms that is not currently well documented but worth implementing if we decide to merge this.
As far as I can see, TrivialAugment is a fairly new method and your are one of the authors of the paper. I've not yet read the paper so I can't comment on the performance improvements it delivers, but typically in the TorchVision repo we avoid adding methods that are recently introduced. Typical criteria for assessing the inclusion of the technique are the number of references of the paper, the popularity of the technique in the community and whether it is considered mature enough so that we can provide a stable API.
@fmassa Could you share your thoughts on whether you think we should include the technique now or wait?
@SamuelGabriel it would be also very useful to hear your thoughts on how much mature you consider the algorithm and whether you believe you will iterate on it soon. Note that this PR does not have to be "all or nothing". I see that you already have a repo where you offer an official implementation of the algorithm, so perhaps we could modify this PR to include only the refactoring of AutoAugment that you deem necessary so that you can build upon it and extend it on your repo easier.
Looking forward to your input.
@datumbox The TrivialAugment Algorithm is final. I don't foresee any changes to the algorithm in the future. It is the simplest working baseline I can think of, thus it is hard to iterate on. I could also offer to add even more APIs: by implementing RandAugment alongside TrivialAugment, since the refactor allows to implement it with relative ease as well. (But, of course, I am the author of TrivialAugment, and draw my motivation from people using my research findings: I am biased.) |
@SamuelGabriel Thanks for being transparent and for providing additional information. I agree with many of the points you raised. I spoke also with Francisco and we would like a bit more time to investigate aka read the paper thoroughly, do some tests on our side etc. I currently got on the pipeline a few other urgent tasks, so I think I can get back to this in a couple of weeks. Does this work for you? Concerning RandAugment, it's within my plans to add it (see #3817) but if you are interested in sending a separate PR that would be awesome. Just let me know if you start working on it to avoid duplication of effort. Again thanks for sharing your contribution, let's keep the discussion open and decide what's best. |
@datumbox Great! Thanks, that works. I would need the AutoAugment refactorings of this pull-request, though, to make RandAugment simple, so I think it would be the easiest for me to simply add it to this PR when we know how we continue in a few weeks. Would that be good? |
@SamuelGabriel I think it would be best to separate the refactoring from the method so that we can merge them independently. Of course it also depends on your availability, but if you have bandwidth you are welcome to split this PR into two. This would allow us to merge the necessary refactoring ASAP while you implement RandAugment and we do the checks on TrivialAugment. Let me know. :) |
036ff5d
to
425c52d
Compare
@SamuelGabriel I was hoping to get some clarifications concerning your ImageNet experiments at the paper:
Thanks! |
@datumbox Hi Vasilis, Thank you for taking the time. Warning: I am actually on my honey moon this week so I might not reply as fast as I would like to until next week.
I apply CutOut only for non-imagenet experiments, just like the previous methods I compare to. As this is the standard practice.
No, that is not what I meant to say. I use the same pipeline as previous work. So TA_wide is used together with cutout for all CIFAR and SVHN experiments yielding SOTA scores. For ImageNet it is just not a common practice to use Cutout and was previously shown to not improve scores (I do not remember where tbh and saw I didn't put a citation there either). I believe experimenting on this with Imagenet is also hard as one would have to reimplement a baseline as well, since none of them use cutout. I could do it for comparing RA to TA anyways though, as I have an RA implementation ready. Cheers, Sam |
@SamuelGabriel Congratulations, best wishes to both of you! :) No rush/worries, thanks for the information. |
# Conflicts: # references/classification/presets.py # test/test_transforms.py # torchvision/transforms/autoaugment.py
496de93
to
bd2dc17
Compare
21b8fad
to
46f886c
Compare
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.
@SamuelGabriel Thanks a lot for the contribution.
We did some checks on our end as well and we think that this is a good addition, especially for people who work on similar but different datasets than ImageNet. I should also note that your contribution and proposed approach helped us review the API of the augmentation code, so your contribution here is much bigger than just adding the TrivialAugment. Thank you!
As this PR contains also bug fixes for RandAugment, I'll merge this to get things going and we can follow up any further improvements on separate PRs. I just have one question for you below, please let me know.
image. If given a number, the value is used for all bands respectively. | ||
""" | ||
|
||
def __init__(self, num_magnitude_bins: int = 30, interpolation: InterpolationMode = InterpolationMode.NEAREST, |
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.
Should this also be 31 as per your comment at #4348 (comment)?
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.
Yes, it should be. I pushed a commit to change it to the underlying branch, but I am not sure what is the best way to get such a minor fix into main now, see SamuelGabriel@9df8f13
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.
No worries, I'll fix it on #4370.
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.
Great, Thanks!
Oh, I am happy to see that TrivialAugment is now in |
@SamuelGabriel I forgot to share the findings of our runs with you. The TrivialAugment was tested as a drop-in replacement of AutoAugment on the current training recipe of MobileNetV3 and it improved the accuracy by 0.1 acc points. Interestingly enough, using random-erasing (as with the baseline of AA) provides better results than without. We did 3 runs for each setup, below I report the results without weight averaging or EMA:
Note that the above experiment puts TA in a disadvantage comparing to AA because all other hyperparams in the recipe were tuned while using AA. The fact that TA beats it makes it quite impressive in my book. |
…ode (#4221) Summary: * Initial Proposal * Tensor Save Test + Test Name Fix * Formatting + removing unused argument * fix old argument * fix isnan check error + indexing error with jit * Fix Flake8 error. * Fix MyPy error. * Fix Flake8 error. * Fix PyTorch JIT error in UnitTests due to type annotation. * Fixing tests. * Removing type ignore. * Adding support of ta_wide in references. * Move methods in classes. * Moving new classes to the bottom. * Specialize to TA to TAwide * Add missing type * Fixing lint * Fix doc * Fix search space of TrivialAugment. Reviewed By: fmassa Differential Revision: D30793337 fbshipit-source-id: 01ffd0268c10beb7d96017ad9490d3d5c9238810 Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com> Co-authored-by: Vasilis Vryniotis <vvryniotis@fb.com>
I wondered whether you didn't test TA before adding it to the library, I am happy you did and to see that you could use it so successfully out of the box, just like intended. :) |
Dear Torchvision Maintainers,
Thank you for maintaining this very helpful library!
I am the author of the TrivialAugment paper (https://arxiv.org/abs/2103.10158) which I will present as an oral at the upcoming ICCV. The paper proposes a trivial baseline for automatic augmentation methods, which outperforms setups like AutoAugment or RandAugment on all evaluated image classification tasks. Since, it is a trivial baseline, I have the impression it is something solid to start from before thinking about augmentations. Therefore, I believe it makes sense to have it in the vision library. What do you think?
I implemented an initial proposal, including tests, that already uses the even stronger augmentation space setting, the results for which we publish in the update of the paper (
ta_wide
).What I did in this PR:
test/test_transforms_tensor.py
What I did not do so far, but offer to do:
Best regards,
Sam
cc @vfdev-5 @datumbox