-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Move resnet video models to single location #1190
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
# init weights | ||
self._initialize_weights() | ||
|
||
if zero_init_residual: |
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.
Do we want to make this the default behavior, and remove the arg?
Do you know if this changes anything wrt the performance?
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.
Do we want to make this the default behavior, and remove the arg?
Yeah, I think that's reasonable.
and no, haven't tried without it
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.
Looks good to me - could we just verify that the number of params is still constant?
pretrained, progress, | ||
block=BasicBlock, | ||
conv_makers=[Conv2Plus1D] * 4, | ||
layers=[2, 2, 2, 2], |
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.
we should probably add a guide on default configs?
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.
I was thinking that this is how we currently implement it for resnets, and it should be fine in most cases?
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.
I suppose that makes sense - this is easier to hack ;)
# init weights | ||
self._initialize_weights() | ||
|
||
if zero_init_residual: |
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.
Do we want to make this the default behavior, and remove the arg?
Yeah, I think that's reasonable.
and no, haven't tried without it
I just verified and the number of parameters is the same before and after the change. |
Sounds good then :) |
Codecov Report
@@ Coverage Diff @@
## master #1190 +/- ##
==========================================
+ Coverage 65.64% 65.65% +0.01%
==========================================
Files 79 74 -5
Lines 5827 5780 -47
Branches 889 883 -6
==========================================
- Hits 3825 3795 -30
+ Misses 1731 1722 -9
+ Partials 271 263 -8
Continue to review full report at Codecov.
|
Training those models yielded the following accuracies, for a clip length of 16 frames:
Which are pretty close to the reported results. |
This simplifies hacking with the code, as now everything lives in a single place.
I have modified the functionality of the model (at least not on purpose).
I believe previous versions had a slight issue with the number of
conv_builder
blocks for mc_18 (5 instead of 4), but the last one wasn't used so this didn't change anything wrt the model that was generated.I still need to update the trained model weights, which will be done in a follow-up PR.
cc @bjuncek