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

Bug fix: Models with shared layers shouldn't be considered Sequential like #8025

Merged
merged 3 commits into from
Oct 2, 2017

Conversation

farizrahman4u
Copy link
Contributor

As of now, the following model is being considered as a "Sequential-like":

from keras.layers import *
from keras.models import *

a = Input((5,))
dense = Dense(5)
b = dense(a)
c = dense(b)

model = Model(a, c)
model.summary()

@farizrahman4u farizrahman4u changed the title Bug fix: Models with shared layers shouldn't considered Sequential like Bug fix: Models with shared layers shouldn't be considered Sequential like Sep 30, 2017
@farizrahman4u
Copy link
Contributor Author

@fchollet

for layer in model.layers:
flag = False
for node in layer.inbound_nodes:
if node in nodes:
Copy link
Member

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?

Copy link
Contributor Author

@farizrahman4u farizrahman4u Sep 30, 2017

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.

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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.

flag = True
if not sequential_like:
break
del nodes
Copy link
Member

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

Copy link
Member

@fchollet fchollet left a 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.

@fchollet fchollet merged commit 213ec19 into keras-team:master Oct 2, 2017
@farizrahman4u farizrahman4u deleted the summary_fix branch October 2, 2017 21:57
ozabluda pushed a commit to ozabluda/keras that referenced this pull request Oct 2, 2017
… like (keras-team#8025)

* models with shared layers are not sequential

* pep8

* remove del statement
dschwertfeger added a commit to dschwertfeger/keras that referenced this pull request Oct 8, 2017
…-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.
  ...
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.

2 participants