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

Add AvgPool1D layer #97

Merged
merged 6 commits into from
Jun 13, 2021
Merged

Add AvgPool1D layer #97

merged 6 commits into from
Jun 13, 2021

Conversation

mkaze
Copy link
Contributor

@mkaze mkaze commented Jun 3, 2021

Resolves #61. This PR adds support for AvgPool1D layer.

  • Implement layer class
  • Add unit tests for layer implementation
  • Add support for importing models containing this layer from JSON config files
  • Add support for exporting models containing this layer to JSON config files

Copy link
Collaborator

@zaleslaw zaleslaw left a comment

Choose a reason for hiding this comment

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

Remove dataFormat and change the params in constructor

*/
public class AvgPool1D(
public val poolSize: Int = 2,
public val strides: Int? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add here the default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since in pooling layers it's very common for strides to be the same as pool size value, the benefit of using null as default value is that the user won't need to set it explicitly and we just default to pool size value instead. This is the same behavior in Keras and PyTorch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Kotlin and Python users have a different experience with nullability, and Kotlin is trying to be a place without nulls.
As I commented in the MaxPool1D layer PR, please change it to the default value 1,2,1.

We need to keep control of the parameters that could be a hyper-parameter for AutoML tasks and show it to the user.
As a variant - to make a warning if stride and pool are not equal with reminder that it's a common practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I understand your point about removing nullability and hidden dependencies. However, I don't get the part about losing control of the parameter during hyper-parameter tuning scenarios or architecture search. Anyways, since I am new to Kotlin I think you have a much better understanding of what would be the best decision here and I would follow it gladly :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant that we train the user to use only one parameter and leave the second null to change together. Perhaps, looking for a new architecture, a method a la NAS, something with this approach will be missed, at least I think so.

@zaleslaw zaleslaw added the Review This PR is under review label Jun 9, 2021
@mkaze
Copy link
Contributor Author

mkaze commented Jun 10, 2021

@zaleslaw I just merged it with master branch and updated it with recent changes.

@mkaze mkaze requested a review from zaleslaw June 10, 2021 15:17
@mkaze
Copy link
Contributor Author

mkaze commented Jun 11, 2021

@zaleslaw I changed the strides type and default value per your request.

@mkaze mkaze requested a review from zaleslaw June 11, 2021 14:13
@zaleslaw zaleslaw added LGTM PR reviewed and is ready to merge and removed Review This PR is under review labels Jun 11, 2021
@zaleslaw zaleslaw merged commit e6ed4a9 into Kotlin:master Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM PR reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Audio] Add AveragePooling1D layer
2 participants