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

feat: adds ggml_pad_reflect_1d #850

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

balisujohn
Copy link
Contributor

This adds an op that mirrors the behavior or PyTorch's ReflectionPad1d operation.

Implementations for cuda and CPU are provided.

Tests still need to be added, so marking as a draft PR from now.

The cpu version of the op was derived with permission from @PABannier's implementation discussed here: #819

@balisujohn balisujohn marked this pull request as draft June 6, 2024 05:33
@PABannier
Copy link
Contributor

It looks great, @balisujohn. Thanks for starting the implementation! I'm wondering whether we should implement a ggml_pad_reflect and pass in an is_2D argument that handles the 1D and 2D cases as it is done for ggml_im2col.

@balisujohn
Copy link
Contributor Author

I added a test in test-backend-ops and also added test-pad-reflect-1d.cpp which I verified works with and without the #define GGML_USE_CUBLAS, though I'm not sure if the cuda versions of the ops are currently tested in the CI/CD or how to accomplish that.

I think this is ready for review.

@balisujohn balisujohn marked this pull request as ready for review June 14, 2024 08:49
@balisujohn
Copy link
Contributor Author

A test I wasn't sure how to add was "check if it correctly fails when called with a pad length not shorter than ne0 of the input tensor."

@balisujohn
Copy link
Contributor Author

Or and wrt the is_2D argument I support it but I'd ideally like to leave it to a future PR.

@ggerganov
Copy link
Owner

Some similar comments as in the ggml_conv_transpose_1d PR

Regarding the is_2D: currently the im2col is an exception - the only op that uses this pattern. Probably it is better to change im2col to not use it, since the alternative pattern with dimensions suffix (i.e. _1d, _2d, etc.) is well established

@balisujohn
Copy link
Contributor Author

OK ready for review again : ^)

@PABannier
Copy link
Contributor

Hello @slaren @ggerganov ! Do you plan to merge this operation? This would be super helpful for Encodec.cpp to support the Metal backend.

@ggerganov
Copy link
Owner

Hey @PABannier, this PR needs to be updated to the latest ggml changes before merging.

@PABannier
Copy link
Contributor

Hello @balisujohn ! Do you plan to finish this PR soon? Otherwise, I'm happy to finish yours if you give me the permission to push on your fork.

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.

3 participants