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

[WIP] Add SeparableConv1D Layer #157

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dosier
Copy link
Contributor

@dosier dosier commented Jul 8, 2021

Early works for issue:
#125

dosier added 5 commits June 14, 2021 21:21
� Conflicts:
�	api/src/main/kotlin/org/jetbrains/kotlinx/dl/api/inference/keras/ModelSaver.kt
- still have to write tests, and do the 1D variant
- this also fixed a bug in WeightLoader that did not load the bias weights correctly for SeparableConv2D.kt
@dosier
Copy link
Contributor Author

dosier commented Jul 8, 2021

This is still a work in progress but I would appreciate some feedback already if anyone has time :). Will write some tests and implement the 1D version soon.

@dosier dosier changed the title [WIP] Add SeparableConv1D Layer #125 [WIP] Add SeparableConv1D Layer Jul 8, 2021
@dosier dosier marked this pull request as draft July 10, 2021 04:55
*
* Note: layer attributes cannot be modified after the layer has been called once (except the `trainable` attribute).
*
* TODO: add rank for getting the channel axis?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are supporting at this moment only Channels Last (NWHC)

* Note: layer attributes cannot be modified after the layer has been called once (except the `trainable` attribute).
*
* TODO: add rank for getting the channel axis?
* TODO: add docs for params?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, we need docs for params

*
* TODO: add rank for getting the channel axis?
* TODO: add docs for params?
* TODO: add trainable param?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean here? Please explain or provide an example?

protected open fun getOutputDepth(numberOfChannels: Long): Long = filtersInternal

private fun computeMatricesShapes(numberOfChannels: Long) {
depthwiseKernelShape = shapeFromDims(*kernelSizeInternal, numberOfChannels, depthMulitplierInternal.toLong())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better to make the function without side-effects, which returns a triple of shapes

@@ -32,13 +32,13 @@ internal fun depthwiseConv2dBiasVarName(name: String) = name + "_" + "depthwise_
internal fun depthwiseConv2dKernelVarName(name: String) = name + "_" + "depthwise_conv2d_kernel"

/** Default SeparableConv2d bias variable name in TensorFlow graph, based on variable's name. */
internal fun separableConv2dBiasVarName(name: String) = name + "_" + "separable_conv2d_bias"
internal fun separableConvBiasVarName(name: String, dim: Int) = name + "_" + "separable_conv2d_bias"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dim parameter is not used at this moment.. Are you going to create separate variable names depending on dims? Isn't it?

@zaleslaw
Copy link
Collaborator

zaleslaw commented Aug 3, 2021

So @dosier if you have any questions, ask me. I tried to comment on the existing solution and answer a few questions (and made more:)

Great to find any existing NN architecture which uses this layer to attach in comments

@zaleslaw zaleslaw added the Review This PR is under review label Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review This PR is under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants