-
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
Detection recipe enhancements #5715
Conversation
💊 CI failures summary and remediationsAs of commit 162f76d (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 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.
Adding a few comments to clarify what is what.
elif opt_name == "adamw": | ||
optimizer = torch.optim.AdamW(parameters, lr=args.lr, weight_decay=args.weight_decay) | ||
else: | ||
raise RuntimeError(f"Invalid optimizer {args.opt}. Only SGD and AdamW are supported.") |
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.
Straight copy-paste from classification.
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.
since is copy-pasted I think it can stay as is and if we want to change we can do in a different PR, but still wonder if there is a reson why for sgd we use starts_with
and for adamw we use ==?
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.
You are right! I've indeed copy-pasted it from classification but replaced the previous sgd optimizer line. The problem is that the existing recipe didn't contain the nesterov momentum update. I've just updated the file to support it; it's not something I used so far but it's a simple update.
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.
LGTM!
elif opt_name == "adamw": | ||
optimizer = torch.optim.AdamW(parameters, lr=args.lr, weight_decay=args.weight_decay) | ||
else: | ||
raise RuntimeError(f"Invalid optimizer {args.opt}. Only SGD and AdamW are supported.") |
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.
since is copy-pasted I think it can stay as is and if we want to change we can do in a different PR, but still wonder if there is a reson why for sgd we use starts_with
and for adamw we use ==?
@@ -64,7 +64,6 @@ def test_get_weight(name, weight): | |||
) | |||
def test_naming_conventions(model_fn): | |||
weights_enum = _get_model_weights(model_fn) | |||
print(weights_enum) |
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.
profit!
Summary: * Detection recipe enhancements * Add back nesterov momentum Reviewed By: NicolasHug Differential Revision: D35393165 fbshipit-source-id: 32156eddebfcb71e708be2b2a7b7ccef1e48c88e
Fixes #5307
This PR cherrypicks some of the improvements from #5444 to simplify review.
Improve the existing detection recipes:
--opt
and--norm-weight-decay
support in DetectionInstanceNorm
andLocalResponseNorm
layers insplit_normalization_params