-
Notifications
You must be signed in to change notification settings - Fork 443
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
Refactoring ConvModule
by removing act_cfg
#3809
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.
LGTM!
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.
Thanks for your work! I left some comments. Please take a look.
I've canceled the action of this PR due to issues with the integration test at the moment, please update when this PR is merged. #3815 |
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.
Thank you for the update. I have a general question.
Can't we pass and use already instantiated activation instead of activation class (partial)?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3809 +/- ##
========================================
Coverage 80.66% 80.67%
========================================
Files 272 272
Lines 27477 27459 -18
========================================
- Hits 22165 22153 -12
+ Misses 5312 5306 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I considered this before, but I haven't proceeded for now because original |
I actually think this approach when reviewing norm refactoring but it also can be activation layer. I think current approach is ok but Kirill's opinion is also good. |
I'll try this targeting the next version as a new minor feature, conv and normalization can be also appiled. |
Summary
This PR includes:
act_cfg
withactivation_callable
build_activation_layer
How to test
Checklist
License
Feel free to contact the maintainers if that's a concern.