-
Notifications
You must be signed in to change notification settings - Fork 278
Add new CSPNet preset and add manual padding. #2212
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
base: master
Are you sure you want to change the base?
Conversation
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.
@sachinprasadhs code look good except for one minor comment. Are we sure this works with the older cspnet presets?
self.converter = convert_resnet | ||
elif "csp" in architecture: | ||
elif architecture in ( |
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.
can we just flip the order of the if and else if? This will be hard to maintain.
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.
Actually maybe better would be to switch all these checks to architecture.startswith("restnet")
.
You would need two starts with, darknet and csp, joined with an and I guess
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.
Makes sense, this would be lot better, made the changes. Thank you.
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! Just one final comment.
@@ -16,9 +16,9 @@ class TimmPresetLoader(PresetLoader): | |||
def __init__(self, preset, config): | |||
super().__init__(preset, config) | |||
architecture = self.config["architecture"] | |||
if "resnet" in architecture: | |||
if architecture.startswith("resnet"): |
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.
Can you actually make the startswith
change for every elif
below? I think it will be a safer and less confusing way to check this overall.
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.
Changed
-inf
instead ofzeropadding
beforemaxpooling
to handle edge case scenario to consider negative values from Leaky Relu activation output.