-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
Bug fix: Models with shared layers shouldn't be considered Sequential like #8025
Conversation
for layer in model.layers: | ||
flag = False | ||
for node in layer.inbound_nodes: | ||
if node in nodes: |
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.
Wouldn't this always be True?
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.
Not necessarily. A layer might be shared between 2 models and hence have more than one inbound node. But we are looking for layers which are being shared within the same model. The inbound nodes should be part of the graph of the model for which we are printing the summary.
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.
dense = Dense(10)
a = Input((10,))
b = dense(a)
model1 = Model(a, b)
a2 = Input((10,))
b2 = dense(a2)
model2 = Model(a2, b2)
model1.summary()
# Here dense layer has 2 inbound nodes,
#but the second node is not part of model1.
# So should be considered sequential like
if (len(v) > 1) or (len(v) == 1 and len(v[0].inbound_layers) > 1): | ||
# if the model has multiple nodes or if the nodes have multiple inbound_layers | ||
# the model is no longer sequential | ||
sequential_like = False | ||
break | ||
nodes += v | ||
if sequential_like: | ||
# search for shared layers |
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.
I don't get the criteria you use for shared layers. A comment here would help
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.
A layer is shared in a model if it has 2 or more inbound nodes and those inbound nodes are part of the graph.
keras/utils/layer_utils.py
Outdated
flag = True | ||
if not sequential_like: | ||
break | ||
del nodes |
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.
This doesn't seem necessary
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, thanks
Normally we'd need a unit test for correctness (since this is non-obvious code) but in this case we don't have unit tests for this function, unfortunately.
… like (keras-team#8025) * models with shared layers are not sequential * pep8 * remove del statement
…-outputs * master: (68 commits) Change default value of shuffle parameter of Sequential.fit_generator() from True to False. (keras-team#8075) Fix off-by-one bug in predict/evaluate progress bar (keras-team#8071) Revert "Faster sequence" (keras-team#8060) Support NCHW for conv2d. (keras-team#8021) Change compute_accuracy() argument order and names (keras-team#8049) Replace literal constant 10 with variable num_classes in example/ (keras-team#8041) Faster sequence (keras-team#8039) Improve RNN docs. Enable accuracy reporting during training in examples/mnist_siamese_graph.py (keras-team#7997) Bug fix: Models with shared layers shouldn't be considered Sequential like (keras-team#8025) Add 'subtract' merge layer documentation (keras-team#8038) Update inference in seq2seq script to be more efficient Remove lstm_benchmark from examples/README.md (keras-team#8024) Add shuffle to the Model API (keras-team#8023) Add seq2seq example script. fix travis failure (keras-team#8014) Improve TF backend's Switch function (keras-team#7958) Added support for dynamic noise_shape in Dropout (keras-team#7999) Make on_epoch_end optional (keras-team#8007) Incremental tests speed ups. ...
As of now, the following model is being considered as a "Sequential-like":