-
Notifications
You must be signed in to change notification settings - Fork 124
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
Replace add_missing_layers with add_missing_container_layers #169
Conversation
Instead, use add_missing_container_layers
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## main #169 +/- ##
==========================================
- Coverage 97.39% 97.08% -0.31%
==========================================
Files 6 6
Lines 575 584 +9
==========================================
+ Hits 560 567 +7
- Misses 15 17 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thanks for the PR! I'll need some time to look at the code more closely but the general direction is correct - a more specific add_missing_container_layers function helps readability and future debugging a lot. I left a comment on some of the output changes. Most of them are better but one seems worse (missing MaxPool / PReLU layers, just need some clarification there - are they not used in forward at all? Only used in train mode?) One pre-commit hook is failing, feel free to use |
Looks correct to me, thanks for the contribution! I also added some additional tests to ensure it solves the problems it sets out to achieve. |
Just realized another potential improvement:
So, I think it is no longer necessary to keep updated hierarchy for parent modules since all the parents (including container modules) are already in the summary list. |
Yep, simplifying that code and making layers_to_str extra simple sounds like a win to me! |
Hi,
I in this PR, I tried to get rid of
add_missing_layer
and makesummary
primarily based on forward call.There was a problem if you do not include
add_missing_layer
. It turns out that if you only use forward (i.e. noadd_missing_layer
), container modules are being ignored and not included insummary
list. This leads to one problematic situation due to wrong parameter count. For instance see below result (obtained withoutadd_missing_layer
)In terms of layer config, there is no problem since this is handled by the function
layers_to_str
informatting.py
. But, the parameter count is wrong since leftover parameters for the entire GenotypeNetwork is 3,200, but it should be 0 --. This occurs becauseModuleList: 1
is not contained in summary_list, andCell: 2-3
is considered to be children ofSequential: 1-1
. To resolve this, I added the functionadd_missing_container_layers
. This adds the container Modules (e.g. ModuleDict or ModuleList) used in main module. Once this is used, the result for the same case is correct:In addition to making summary based on forward pass, which as far as I can tell is beneficial, this resolves the discrepancies about the ordering in module
__init__
and ordering of modules inforward
. For instance, after this commit, the following test case gives the intended result. This used to be a problem, to be shown below. Note the order in which modules inself.block0
is defined is given byrange_1
.The result before commit (with add_missing_layer included), param count is wrong, it should not be 816:
The resultant summary after commit is,:
Note: As you may realize, I used the tracing algorithm in
layers_to_str
functionformatting.py
to obtain container modules inadd_missing_container_layers
.Looking forward to feedbacks