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 AvgPool3D layer #99

Merged
merged 6 commits into from
Jun 13, 2021
Merged

Add AvgPool3D layer #99

merged 6 commits into from
Jun 13, 2021

Conversation

mkaze
Copy link
Contributor

@mkaze mkaze commented Jun 3, 2021

Resolves #81. This PR adds support for AvgPool3D 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

NOTE: as commented in the unit tests, currently it gives an error when dataFormat = CHANNELS_FIRST.

*/
public class AvgPool3D(
public val poolSize: IntArray = intArrayOf(2, 2, 2),
public val strides: IntArray? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

set the default value for the strides and convert both, strides and poolSize to the 5d array of longs

Copy link
Contributor Author

@mkaze mkaze Jun 10, 2021

Choose a reason for hiding this comment

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

Conversion was done, and I have mentioned the benefit of using null as the default value for strides.

@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/requested changes.

@mkaze mkaze requested a review from zaleslaw June 10, 2021 15:54
arrayOf(
arrayOf(
arrayOf(
floatArrayOf(18.0f/8, 4.5f/8, 16.0f/8),
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you take these values from Keras tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I created the test data myself :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not bad:)
Sometimes I just copied test data from Keras; sometimes, if test data seems not appropriate, I use Keras like a test machine writing some simple test in Python to put some test data in inputs and get outputs. A few times, I ignored this method and just used tested primitive itself to get data, but it has a bug in implementation and, as a result, a wrong test:(( But I agree that test inputs/outputs for pooling could be calculated manually (just shared my experience)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, actually I have created test data for all test cases I have written so far; but certainly it's not a sustainable approach and using another tested tool, like Keras or maybe even numpy, for generating test data seems to be a better and more reliable approach.

@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:28
@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 218fb57 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.

[Video] Add AveragePooling3D layer
2 participants