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

Give devs the flexibility to control PointwiseAvgPoolAntialiased2D output #36

Open
psteinb opened this issue Mar 3, 2023 · 1 comment

Comments

@psteinb
Copy link
Contributor

psteinb commented Mar 3, 2023

Just bumped my head into this issue for a while during debugging. It would be great if the implicit kernel_size could be made an explicit part of the API of PointwiseAvgPoolAntialiased2D and others:

class PointwiseAvgPoolAntialiased2D(EquivariantModule):

    def __init__(self,
                 in_type: FieldType,
                 sigma: float,
                 stride: Union[int, Tuple[int, int]],
                 # kernel_size: Union[int, Tuple[int, int]] = None, #<- this provides only rigid control of this class
                 padding: Union[int, Tuple[int, int]] = None,
                 #dilation: Union[int, Tuple[int, int]] = 1,
                 ):

I suggest to make kernel_size part of the API again and fill filter with 0 for example. This would give downstream users the freedom to play more with the output of the underlying conv operation.

@Gabri95
Copy link
Collaborator

Gabri95 commented Mar 8, 2023

Hi @psteinb

I am not completely convinced by this solution, to be honest.

First, at the moment, the kernel_size is chosen to fit best the antialiasing filter.
I feel like allowing the user to set a different kernel_size would make this module less reliable, since a bad choice of kernel_size will break the antialiasing filter without the user noticing.

Second, the PointwiseMaxPoolAntialiased2D module already has a kernel_size argument which, however, has a different meaning (it refers to the size of the window used to compute the max).
I fear having a kernel_size argument in the average pooling would be confusing.

I will probably use a different naming for this kernel_size and raise a warning if it is too small. I'll try to implement this in the next days, but feel free to open a PR if you already did something like that

Best,
Gabriele

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

No branches or pull requests

2 participants