Skip to content

Add more padding variations: cycle, replicate and mirror#1719

Open
laszlokorte wants to merge 4 commits intoelixir-nx:mainfrom
laszlokorte:cycle-padding
Open

Add more padding variations: cycle, replicate and mirror#1719
laszlokorte wants to merge 4 commits intoelixir-nx:mainfrom
laszlokorte:cycle-padding

Conversation

@laszlokorte
Copy link
Copy Markdown

Until now only constante padding (Nx.pad/3) and reflection padding Nx.reflect/2 are supported by Nx.

This pull request adds Nx.cycle/2 for cyclic padding, Nx.replicate/2 for using the outmost value along each axis as pad value and Nx.mirror/2 which behaves similar to Nx.reflect/2 but does repeat the edge values.

I did already propose especially the cyclic padding in #1708

Until now only constante padding (`Nx.pad/3`) and reflection padding
`Nx.reflect/2` were supported.
This commits add `Nx.cycle/2` for cyclic padding, `Nx.replicate/2` for
using the outmost value along each axis as pad value and `Nx.mirror/2`
which behaves similar to `Nx.reflect/2` but does repeat the edge values.
Copy link
Copy Markdown
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR!

Let's also add some tests (maybe in nx_test.exs?) that stress test some of the edge cases, such as negative padding config (Nx.pad does slicing in the case of negative padding, maybe these need to do the same? Do we have Numpy references for these?)

nx/lib/nx.ex Outdated
Comment on lines +17095 to +17096
left = Nx.subtract(n - 1, Nx.iota({n}))
right = Nx.iota({n})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
left = Nx.subtract(n - 1, Nx.iota({n}))
right = Nx.iota({n})
right = Nx.iota({n})
left = Nx.subtract(n - 1, right)

we might need to do something a bit more complicated here, in that iota being a creation function might need to receive the backend opts extracted from the input argument

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim do you think this is something we should be concerned about?

nx/lib/nx.ex Outdated
]
>
"""
@doc type: :shape
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim WDYT we add type: :padding instead and move some of the previous funs there as well?

@josevalim
Copy link
Copy Markdown
Contributor

Sorry, I originally thought we were adding only a single function, but after seeing we need multiple and looking at the implementation (thank you!), I think we should go ahead and pass the mode as second argument to pad, as done for numpy: https://numpy.org/doc/stable/reference/generated/numpy.pad.html (using same names too). Especially because the padding_config is still a required argument. In this scenario, we would deprecate Nx.reflect/2.

For example: Nx.pad(tensor, :reflect, padding_config), Nx.pad(tensor, :symmetric, padding_config). Plus numpy has a few other modes we may want to add, so this will avoid adding 7-8 different functions.

@laszlokorte
Copy link
Copy Markdown
Author

laszlokorte commented Apr 1, 2026

I agree that unifying with pad/3 would be much cleaner.

@laszlokorte
Copy link
Copy Markdown
Author

I have update the code to unify the 4 padding styles into the pad/3 function. currently the padding_config for pad/3 expected {left, right, interior} for the constant padding but the reflect/2 function - that I based the other padding styles on - expects {left, right}, not supporting interior padding.

I am not sure what makes most sense API-wise. Just document that the padding_config format differs depending on padding type or come up with some sensible value for interior values (just constant 0?) for the other padding types.

@josevalim
Copy link
Copy Markdown
Contributor

Ah, if they have different padding_config then it may make sense to give the function a different name. Overall, I think this approach is better, we just need to figure out the name.

@laszlokorte
Copy link
Copy Markdown
Author

I agree that making the padding_config structure vary by padding type is confusing but a unifying approach could be:
For constant padding allow {left, right, interior} (as before) as well as {left, right} (new) defaulting {left, right, 0}
and for the other padding types also allow both {left, right, interior \ 0} and {left, right} but just using 0 as constant interior value.

Alternative names could be pad_outside or pad_around but at a first glance I do not like either.

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