Add more padding variations: cycle, replicate and mirror#1719
Add more padding variations: cycle, replicate and mirror#1719laszlokorte wants to merge 4 commits intoelixir-nx:mainfrom
Conversation
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.
polvalente
left a comment
There was a problem hiding this comment.
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
| left = Nx.subtract(n - 1, Nx.iota({n})) | ||
| right = Nx.iota({n}) |
There was a problem hiding this comment.
| 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
There was a problem hiding this comment.
@josevalim do you think this is something we should be concerned about?
nx/lib/nx.ex
Outdated
| ] | ||
| > | ||
| """ | ||
| @doc type: :shape |
There was a problem hiding this comment.
@josevalim WDYT we add type: :padding instead and move some of the previous funs there as well?
|
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 For example: |
|
I agree that unifying with |
|
I have update the code to unify the 4 padding styles into the 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 |
|
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. |
|
I agree that making the padding_config structure vary by padding type is confusing but a unifying approach could be: Alternative names could be |
Until now only constante padding (
Nx.pad/3) and reflection paddingNx.reflect/2are supported by Nx.This pull request adds
Nx.cycle/2for cyclic padding,Nx.replicate/2for using the outmost value along each axis as pad value andNx.mirror/2which behaves similar toNx.reflect/2but does repeat the edge values.I did already propose especially the cyclic padding in #1708