Skip to content

Poolings #214

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Poolings #214

wants to merge 1 commit into from

Conversation

ricor07
Copy link
Collaborator

@ricor07 ricor07 commented Mar 14, 2025

Starting to create PR. Still a stage 0 draft

@ricor07
Copy link
Collaborator Author

ricor07 commented Mar 17, 2025

@milancurcic just a brief question: if maxpool2d requires input3d this means that I can't write a maxpool3d without an input4d? (Which of course I'm not going to implement)

@milancurcic
Copy link
Member

I don't know. What input rank does maxpool3d need and what does it output?

@ricor07
Copy link
Collaborator Author

ricor07 commented Mar 17, 2025

Yes I noticed I shouldn't have written it. Maxpool3d requires a input4d and outputs a 4d array

@milancurcic
Copy link
Member

I think that's correct. For maxpool2d naming I merely followed an existing convention (in Keras I think) where 2d refers to pooling over 2d, with an extra dimension for channels. If we follow the same for maxpool3d, it would be for 4d total, as you write. However I'm not familiar with applications that pool over 3d, so I leave it to you to determine if this is a common/needed use case.

@ricor07
Copy link
Collaborator Author

ricor07 commented Mar 17, 2025

I may be wrong, but I fear that in neural-fortran you called maxpool1d what in tensorflow is called maxpool2d (https://www.tensorflow.org/api_docs/python/tf/keras/layers/MaxPool2D) and neural-fortran's maxpool2d is tensorflow's maxpool3d. In this way, we have no tensorflow's maxpool1d. Can you please have a look? I'm not sure of this

@ricor07 ricor07 marked this pull request as ready for review March 18, 2025 17:27
@ricor07 ricor07 marked this pull request as draft March 19, 2025 16:27
@milancurcic
Copy link
Member

I think I understand the confusion now. The naming is consistent with Keras. What makes it seem inconsistent is that in neural-fortran maxpool2d I never implemented setting different pool sizes in x- and y-dimensions, and pool_size is an integer, so you can only do square-shaped pools.

From the above linked docs:

Downsamples the input along its spatial dimensions (height and width) by taking the maximum value over an input window (of size defined by pool_size) for each channel of the input.

This means that Keras's MaxPool2D takes 3D input and produces 3D output. Neural-fortran's maxpool2d does the same.

@ricor07
Copy link
Collaborator Author

ricor07 commented Mar 26, 2025

Hello. Any updates from your side? Sorry for not having given progresses but these days i'm very busy.

@milancurcic
Copy link
Member

Thanks @ricor07 for the effort so far and no rush. Whenever you have time. I've been busy with travel past week or so but am otherwise focused on reviewing other open PRs. Is there something specific about this one that you'd like me to look at? In general I don't review PRs until they're marked ready for review (i.e. not a draft).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants