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

Circular padding in convolutional neural networks #971

Closed
VolodyaCO opened this issue Feb 1, 2021 · 4 comments · Fixed by #1661
Closed

Circular padding in convolutional neural networks #971

VolodyaCO opened this issue Feb 1, 2021 · 4 comments · Fixed by #1661
Labels
Priority: P2 - no schedule Best effort response and resolution. We have no plan to work on this at the moment. Status: pull requests welcome We agree with the direction proposed, feel free to give it a shot and file a pull request

Comments

@VolodyaCO
Copy link

Description of the model to be implemented

In many areas such as physics, it is convenient to have convolutional layers with periodic boundary conditions (e.g. see netket)

Therefore, it would be nice to add a "CIRCULAR" padding option to convolutional layers, just as they do in neural-tangents.

Dataset the model could be trained on

1D or 2D data. Maybe MNIST images.

Specific points to consider

None in particular. Just as an example, suppose that one has the 1D data [1,2,3,4,5] and one has filters of size 3, and a stride of 3. The idea is then that two filter operations are carried out. The first one will use [1,2,3], and the second one will use [4,5,1].

Reference implementations in other frameworks

neural-tangents has replaced stax's GeneralConv by a Conv layer, which has this padding option, and further does not require to provide directly the XLA's dimension_numbers.

This was referenced Feb 1, 2021
@marcvanzee
Copy link
Collaborator

I think it would be quite nice to add this, since it doesn't seem to complicate the API much (no additional parameters etc). @levskaya what do you think of this proposal? I recall you were involved in a discussion around this before, and I'm curious whether you think it makes sense to add this.

@marcvanzee marcvanzee added Priority: P2 - no schedule Best effort response and resolution. We have no plan to work on this at the moment. Status: pull requests welcome We agree with the direction proposed, feel free to give it a shot and file a pull request labels Feb 1, 2021
@jheek
Copy link
Member

jheek commented Feb 2, 2021

It would be even nicer if the jax conv op would support this out of the box. They already have 'same' and 'valid'.

@sgrigory
Copy link
Contributor

sgrigory commented Nov 3, 2021

If this is still relevant, I'd be happy to raise a PR, reusing the code from #903 (comment) and adding some tests

@VolodyaCO
Copy link
Author

VolodyaCO commented Nov 4, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P2 - no schedule Best effort response and resolution. We have no plan to work on this at the moment. Status: pull requests welcome We agree with the direction proposed, feel free to give it a shot and file a pull request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants