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

Conv padding: remove string values, move to TF-style values #62

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stevenabreu7
Copy link
Collaborator

As pointed out by Terry, there is no need to support string padding values such as "same" or "valid" as "valid" is equivalent to padding=0, and "same" cannot always be implemented correctly using the PyTorch-style padding that we're currently using.

Question: should we move to Tensorflow-style padding, where the padding at the start and end of each dimension may be different? I.e. instead of (pad_x, pad_y), we specify ((pad_top, pad_bottom), (pad_left, pad_right))?

@matjobst
Copy link
Collaborator

I don't really see the issue here about the string paddings. Would you like the string padding values left out for reduced complexity?
Regarding the four-sided padding: That is more general than the PyTorch style. So I would be in favor of supporting it.

@stevenabreu7
Copy link
Collaborator Author

Yeah, supporting string padding is not really necessary (it's a higher level of abstraction, and an IR is not the place for that), plus it adds more complexity. It also becomes a bit arbitrary what string values we support - there are many: same, valid, full, wrap, reflect, constant, mirror, nearest, etc.

I also agree with TensorFlow-style padding. We can still allow for PyTorch-style padding, and just cast that to the TensorFlow-style in the __post_init__ (similarly to how we already do it to allow integer padding for Conv2D).

@stevenabreu7 stevenabreu7 changed the title Remove string padding values Conv padding: remove string values, move to TF-style values Oct 18, 2023
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