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
Allow applying on all modules, not just immediate children #10
base: main
Are you sure you want to change the base?
Conversation
Hi @hub-bla . Thank you for your contribution. Since freeze_modules accepts regex, I'm wondering if a more flexible regex could select nested modules with the existing code? For example, the following regex seems to work for import argparse
from collections import OrderedDict
from torch import nn
from corenet.modeling.misc.common import freeze_modules_based_on_opts
opts = argparse.Namespace(**{"model.freeze_modules": r"model1(.*)\.conv1"})
inside_model2 = nn.Sequential(
OrderedDict([
('conv1', nn.Conv2d(1,20,5)),
('relu1', nn.ReLU()),
('conv2', nn.Conv2d(20,64,5)),
('relu2', nn.ReLU())
])
)
inside_model = nn.Sequential(
OrderedDict([
('ins_model', inside_model2),
('conv1', nn.Conv2d(1,20,5))
])
)
model = nn.Sequential(
OrderedDict([
('model1', inside_model),
('conv1', nn.Conv2d(20,64,5)),
('conv2', nn.Conv2d(20,64,5))
])
)
print(freeze_modules_based_on_opts(opts, model)) |
Hi @mohammad7t, I agree that existing code can support that operations using loop on named_parameters. To be honest I didn't checked if it works before I started implementing the enhancement. I followed a comment that is above the loop with named_children. To be honest I wonder if that loop with named_modules is even neccessary beacuase everything could by done using as you provided. It would also remove this issue |
I see where you are coming from! I agree that the corenet/corenet/modeling/misc/common.py Lines 200 to 208 in aaa14a6
That's a good question. I think the only reason we need the loop with named_modules is to apply I'm not entirely sure, what the best solution is right now. Let me think a bit more and get back to you. Thinking loudly, I guess we don't need to support the ">" css operator, but the bfs is probably a good idea to address the TODO. What do you think? Thanks again! |
Thank you for clarification! Now, I get it and I agree with everything you said. Regarding the ">" selector, I'm not sure if it's unnecessary when applying bfs. The way it works now is that the input string is splitted by this symbol and then those chunks that might be a regex expression or not, are then passed to bfs. Without it, we had to split the string by dot symbol which is a special character in regex and this might cause some problems. I'll try to think about that too. |
I've made nested module selection based on the way the CSS children selector works. By using '>' we can now select nested modules.
Example:
returns: