Skip to content
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

Fix subset param import #412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix subset param import #412

wants to merge 1 commit into from

Conversation

curufinwe
Copy link
Collaborator

Fix bug where custom_parameter_importer = "subset" would not operate on parameters inside rec layers.

@albertz
Copy link
Member

albertz commented Dec 16, 2020

See failing test, i.e. code style.

@curufinwe
Copy link
Collaborator Author

There is one remaining warning that is not easily fixable: I import _SubnetworkRecCell to do an isinstance check. Can this warning be ignored or how should I fix that?

@albertz
Copy link
Member

albertz commented Dec 16, 2020

Actually, we should not do it this way.
We should introduce more generally a function iter_sub_layers on LayerBase. And this should be overridden by SubnetworkLayer and RecLayer.
Also, _SubnetworkRecCell actually internally has several subnetworks, and you probably should iterate through all of them. Specifically, output_layers_net, input_layers_net and net. See get_layer_from_outside. At least for this use case, you probably want all. Btw, I'm not exactly sure whether there can be duplicates, and whether you want to care about that (keep track in some visited = set() var).

Note that this is a bit inconsistent to the behavior of get_sub_layer, though, which requires that the layer (output) also can be used (which is not the case for layers within a loop). Not sure if we should maybe introduce two separate functions iter_all_sub_layers vs iter_sub_layers or so. Or just do not care about that. TF would anyway tell you when you use it wrongly. So I would vote to just implement iter_sub_layers and iterate all.

returnn/tf/network.py Outdated Show resolved Hide resolved
returnn/tf/network.py Outdated Show resolved Hide resolved
returnn/tf/network.py Outdated Show resolved Hide resolved
returnn/tf/network.py Outdated Show resolved Hide resolved
returnn/tf/network.py Outdated Show resolved Hide resolved
@albertz
Copy link
Member

albertz commented Jan 20, 2021

What is the state here?

@albertz
Copy link
Member

albertz commented Apr 20, 2021

Note: There is now the function get_all_layers_deep, which can be used instead of iter_layers_recursively.

@albertz
Copy link
Member

albertz commented Apr 21, 2021

Ok, I removed iter_layers_recursively now, and just used get_all_layers_deep.

@curufinwe @michelwi can you check it's ok, so that we can merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants