-
Notifications
You must be signed in to change notification settings - Fork 653
Fit DenseNet examples to Keras-2 #53
Fit DenseNet examples to Keras-2 #53
Conversation
keras-team/keras#6034 may be relevant. Also a much more complete version of these changes is in https://github.com/farizrahman4u/keras-contrib/pull/46/files#diff-7c92da79e0a644cc9e206dab6de13bab |
LGTM, thanks! |
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.
@farizrahman4u please revert this change. It is broken. See: a6c7c91#r108550331
My changes will be ready for review today which are a superset of these changes.
@@ -375,15 +376,16 @@ def __dense_block(x, nb_layers, nb_filter, growth_rate, bottleneck=False, dropou | |||
x = __conv_block(x, growth_rate, bottleneck, dropout_rate, weight_decay) | |||
x_list.append(x) | |||
|
|||
x = merge(x_list, mode='concat', concat_axis=concat_axis) | |||
x1 = concatenate(x_list, axis=concat_axis) |
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 good! then all the convolutions will be the same! it is essential that all the concatenation be on x, in other words all new instances of x1 are a bug.
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.
Specifically, this is the source of keras-team/keras#6034
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.
Hmm, interesting. So I should wait for this PR to be merged, right?
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.
Unfortunately, this change both triggers an existing bug and adds a new one. Consider just utilizing/merging #46 it fixes everything mentioned here, and I'll be switching to concat calls once keras-team/keras#6034 or keras-team/keras#6035 is merged.
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.
Sure, I will take a look. Thanks.
This reverts commit fcd054f.
@ahundt Has you code be merged? If yes, then we don't need the revert any more, right? |
@kemaswill I'm still waiting on review for #46 |
Got it. |
No description provided.