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 a flag to allow skipping the first projection in small ResNets #10584

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

zaccharieramzi
Copy link

@zaccharieramzi zaccharieramzi commented Apr 7, 2022

Description

📝 Please include a summary of the change.

  • Please also include relevant motivation and context.
  • List any dependencies that are required for this change.

This should fix #10583
In this issue, I described how the small ResNets implemented here have an extra convolution (for projection), that is neither present in the original paper or in PyTorch.
This PR allows to get rid of this extra convolution with a keyword argument that defaults to the previous behaviour so that this remains a non-breaking change.

Type of change

For a new feature or function, please create an issue first to discuss it
with us before submitting a pull request.

Note: I didn't wait for a discussion because this seemed like a relatively small and simple change,
so it's not bothering for me to have written it even if rejected in the end.

  • New feature (non-breaking change which adds functionality)

Tests

📝 Please describe the tests that you ran to verify your changes.

  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Test Configuration:

I added tests to check the parameter count.
I also offline checked the number of non-trainable parameters to checked that it matched against PyTorch's one.

Checklist

Copy link
Contributor

@arashwan arashwan left a comment

Choose a reason for hiding this comment

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

Thank you!

official/vision/modeling/backbones/resnet.py Show resolved Hide resolved
arashwan
arashwan previously approved these changes Apr 20, 2022
@saberkun saberkun added the ready to pull ready to pull (create internal pr review and merge automatically) label Apr 20, 2022
@saberkun
Copy link
Member

saberkun commented May 7, 2022

There is a failure detected by internal tests. I think we forgot to update configs. @ByzanTine

@zaccharieramzi
Copy link
Author

@saberkun , indeed I had not updated the get_config method: just changed that.

yeqingli
yeqingli previously approved these changes May 24, 2022
@yeqingli yeqingli requested review from yeqingli and removed request for xianzhidu July 22, 2022 17:45
Copy link
Member

@yeqingli yeqingli left a comment

Choose a reason for hiding this comment

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

Please explain the relationship between use_first_projection and self._use_first_projection.

@sineeli sineeli requested a review from yeqingli November 1, 2022 00:02
@yeqingli yeqingli added ready to pull ready to pull (create internal pr review and merge automatically) and removed ready to pull ready to pull (create internal pr review and merge automatically) labels Nov 1, 2022
@laxmareddyp laxmareddyp added the models:official models that come under official repository label Nov 16, 2022
@laxmareddyp laxmareddyp added ready to pull ready to pull (create internal pr review and merge automatically) and removed ready to pull ready to pull (create internal pr review and merge automatically) labels Dec 6, 2023
@laxmareddyp laxmareddyp added ready to pull ready to pull (create internal pr review and merge automatically) and removed ready to pull ready to pull (create internal pr review and merge automatically) labels Jan 25, 2024
@laxmareddyp laxmareddyp added ready to pull ready to pull (create internal pr review and merge automatically) cla: yes and removed ready to pull ready to pull (create internal pr review and merge automatically) labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes models:official models that come under official repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Small ResNets have an extra convolution
6 participants