-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add transpose convolution layers #315
Conversation
I suggest to be an honest library for a user and remove unsupported parameters by adding to the KDoc the NOTE about this removal (with a link to the known bug). But it could be a problem during the model convertation Keras->KotlinDL and KotlinDL->Keras and in the case |
@juliabeliaeva I made an integration example with auto-encoder based on the following article https://www.machinecurve.com/index.php/2019/12/10/conv2dtranspose-using-2d-transposed-convolutions-with-keras/ The Colab is available here, you could a copy of it and download the JSON config and h5 weights put it to examples/resources, and run the following example
It fails with the following error message
Looks like something was missed during the loading of the weights or initialization of the variables. To reproduce the bug, download the following archive (json + h5) |
@zaleslaw thank you, I forgot about loading weights from h5 files completely. I'll implement it. |
That was my concern: if someone loads a model from keras and saves it back, they will loose their parameters in the process. |
62281e3
to
1d5492e
Compare
@zaleslaw I just pushed a new version of this PR. Apart from the added loading weights for transposed convolutions, there are some other notable changes:
|
@juliabeliaeva Is this the final version of this PR or is it required any PR should be merged before? |
It's the final version, can be merged right now. |
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.
Please have a look at the left comments and add the general example related to the autoencoder based on the Colab shared earlier in the issue comments.
No big changes, just a few minor things for future developers.
At this moment #328 is merged to the master, you could merge in this PR and finish the Autoencoder example
@@ -57,42 +56,27 @@ private const val EXTRA_DIM = 1L | |||
* @since 0.3 | |||
*/ | |||
public class Conv1D( | |||
public val filters: Int = 32, | |||
public val kernelSize: Int = 3, |
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.
Looks like kernel size is a stable term in Deep Learning for CNN
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.
We can't use kernelSize
here since kernelSize
is an array and we want to allow passing a single integer here.
} | ||
|
||
override fun toString(): String = | ||
"Conv1D(filters=$filters, kernelSize=$kernelSize, strides=${strides.contentToString()}, " + | ||
"dilation=${dilations.contentToString()}, activation=$activation, kernelInitializer=$kernelInitializer, " + | ||
"biasInitializer=$biasInitializer, kernelShape=${kernel.shape}, biasShape=${bias?.shape}, padding=$padding, " + | ||
"biasRegularizer=$biasRegularizer, kernelRegularizer=$kernelRegularizer, activityRegularizer=$activityRegularizer)" | ||
|
||
internal companion object { |
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.
Should these functions be companion functions or could be just private functions in the Conv1D?
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.
These functions are used in both Conv1D
and Conv1DTranspose
since 1D convolutions are implemented with 2D convolutions. They can't be private, but they are only needed for convolutions implementation, so they are internal.
return kernel.withAdded(EXTRA_DIM - 1, 1) | ||
} | ||
|
||
internal fun Ops.expandKernel(kernel: Operand<Float>): Operand<Float> { |
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.
Is it Conv1D specific functions, could they be a part of our local helper framework over the TensorFlow Java API
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.
These functions are specific for Conv1D
/Conv1DTranspose
, I don't think they are needed outside of these operations.
*/ | ||
public class Conv1DTranspose( | ||
public override val filters: Int = 3, | ||
public val kernelLength: Int = 3, |
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.
Kernel Size is better
override fun convImplementation(tf: Ops, input: Operand<Float>): Operand<Float> { | ||
return tf.withExpandedDimensions(input) { expandedInput -> | ||
val expandedOutputPadding = outputPadding?.withAdded(EXTRA_DIM * 2, listOf(0, 0)) | ||
return@withExpandedDimensions tf.nn.conv2dBackpropInput( |
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.
very complex function call, could we extract some logic to the separate variables and leave some comments for the future (why, for example, we did tf.shapeWithDynamicBatchSize(outputShape.expand(), input)
for the specific parameter and so on) At this moment this knowledge is hidden in the issue comments.
I suggest doing this, because it's enough far from the initial Keras code
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.
why, for example, we did tf.shapeWithDynamicBatchSize(outputShape.expand(), input)
shapeWithDynamicBatchSize
has a javadoc with explanation and links to the relevant issues, I'll add some comments to this method as well.
) | ||
} | ||
|
||
internal companion object { |
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.
Methods of this companion are used in the both, Conv1DTranspose and Conv2DTranspose, probably need we need a separate place with top-level functions a la Util class.
* C - number of channels | ||
* ``` | ||
* | ||
* Note: providing explicit output padding is currently not supported. |
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.
Should we throw an exception on that or add a contract to init
section?
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.
There is no outputPadding
parameter, so there is no situation where we could throw an exception. The note is here since keras does have this parameter (see https://keras.io/api/layers/convolution_layers/convolution3d_transpose/).
} | ||
|
||
internal companion object { | ||
/** |
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.
Could we move here other companion functions mentioned earlier?
@@ -41,7 +41,7 @@ internal fun soundBlock(filters: Int, kernelSize: Int, poolStride: Int): Array<L | |||
arrayOf( | |||
Conv1D( | |||
filters = filters, | |||
kernelSize = kernelSize, | |||
kernelLength = kernelSize, |
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.
kernelSize!
…olution to the convImplementation method
1d5492e
to
0a8a32d
Compare
Pushed a new version of this PR.
I think it might be more convenient to implement the example as another PR. |
This PR adds implementations for
Conv1DTranspose
,Conv2DTranspose
andConv3DTranspose
layers.Caveats:
org.tensorflow.op.NnOps#conv3dBackpropInput
.Conv3DTranspose
still has the parameter, it just does not do anything. I'm not sure if this parameter is needed or if it is, where to add a warning about its usage.Fixes #124