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

[Audio] Add MaxPooling1D layer #60

Closed
zaleslaw opened this issue May 18, 2021 · 4 comments · Fixed by #96
Closed

[Audio] Add MaxPooling1D layer #60

zaleslaw opened this issue May 18, 2021 · 4 comments · Fixed by #96
Assignees
Labels
good second issue Good for advanced contributors
Milestone

Comments

@zaleslaw
Copy link
Collaborator

zaleslaw commented May 18, 2021

We are missing some layers to support the export of models from Keras fully. One of them is the MaxPooling1D layer.

Add a layer class, write documentation for it, write a test for it, try, if possible, create a small trainable network with it (in your own GitHub) and attach a link here in the comments.

The layer should be placed here

As a reference implementation, the MaxPool2D layer could be used, but feel free to improve it!

If some refactoring to pooling layers could be applied, please, try to do it!

Also, support for export and import of layer in JSON format should be added (see ModelLoader.kt and ModelSaver.kt)

A detailed description of the layer can be found here

P.S. There are no-ops for tf.nn.maxPool1d in Java API, so you could try to implement it from scratch via available ops or use tf.nn.maxPool like in TensorFlowJS with reshape in tfjs-core/src/ops/max_pool.ts

@zaleslaw zaleslaw added the good first issue Good for newcomers label May 18, 2021
@zaleslaw zaleslaw added this to the 0.3 milestone May 18, 2021
@zaleslaw zaleslaw added good second issue Good for advanced contributors and removed good first issue Good for newcomers labels May 20, 2021
@mkaze
Copy link
Contributor

mkaze commented Jun 2, 2021

I'll take this and work on it.

@mkaze
Copy link
Contributor

mkaze commented Jun 2, 2021

@zaleslaw A related question: I was wondering why you decided to include batch and channels dimension in the pooling and strides properties of MaxPool2D layer.

https://github.com/JetBrains/KotlinDL/blob/7e8c7d996d259ae1ebd1c102681c49070249bd26/api/src/main/kotlin/org/jetbrains/kotlinx/dl/api/core/layer/pooling/MaxPool2D.kt#L28-L30

Is there any common use case for pooling over those two dimensions? I am asking this because I think including and expecting them in the API, which is supposed to be a high-level API for DL, is mostly redundant and a bit confusing especially for newbie users. Also, neither of Keras or PyTorch follow this approach. We can also simply expect an IntArray of size 2, and then augment it internally and pass it to the low-level API. What do you think?

@mkaze mkaze mentioned this issue Jun 2, 2021
4 tasks
@zaleslaw
Copy link
Collaborator Author

zaleslaw commented Jun 3, 2021

@mkaze The reason was simple - to keep the ability to control all inputs parameters available in low-level Java API
A few contributors (including @avan1235) raised this question due to the difference between this API and Keras/Torch APIs.

I agree that these 4d arrays should be revisited, but revisited together in the same manner. I suggest keeping consistency at this moment.

NOTE: This API is not the final high-level API, we are going to create more high-level Kotlin DSL at the top, there nice to have strides = 2 or pool=2 as one number (because it's very rare or never to use 2 different numbers here)

@mkaze
Copy link
Contributor

mkaze commented Jun 4, 2021

I agree that these 4d arrays should be revisited, but revisited together in the same manner. I suggest keeping consistency at this moment.

Ok, then I think I have to revisit the four PRs I have made for pooling layers to make them accept the full version of pool size and strides instead.

NOTE: This API is not the final high-level API, we are going to create more high-level Kotlin DSL at the top

Nice!

there nice to have strides = 2 or pool=2 as one number (because it's very rare or never to use 2 different numbers here)

I see. For that specific use case, currently I handle that in my PRs by setting strides as nullable and when it's null (which is the default) then it will have the same value as pool size. This is also the approach Keras has taken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good second issue Good for advanced contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants