JuliaImages / ImageFiltering.jl

Julia implementations of multidimensional array convolution and nonlinear stencil operations
Other
99 stars 51 forks source link

padarray inconsistent behavior for n=1 axes #259

Open andrew-saydjari opened 1 year ago

andrew-saydjari commented 1 year ago

I found the following surprising.

n = 0
x = rand(50,100);
xr = repeat(x,outer=[1 1 n])
println(size(xr))
pxr = ImageFiltering.padarray(xr,ImageFiltering.Pad(:reflect,(5,5,0)));
println(size(pxr))

For n!=1 this succeeds and pads the repeated matrix which is of dimension (50,100,n) to be (60,110,n). For n=1, this fails with the following error message.

"DimensionMismatch: supplied axes do not agree with the size of the array (got size (1,) for the array and (0,) for the indices"

Maybe there is something I am not understanding, but I would have expected the n=1 case to behave similarly.

andrew-saydjari commented 1 year ago

A similar problem just came up in another code base in a fairly different application. The handling of zero padding on singleton dimensions seems require (sometimes difficult) special handling, especially if you need shape stability.

ImageFiltering.padarray(zeros(1,100),ImageFiltering.Pad(:reflect,(0,1)))

andrew-saydjari commented 1 year ago

The following seems to be a fix. Not sure why .= has different behavior than copy!(). Does someone who understands the code base here better know why that is different and if the change to .= below would be acceptable/worth a PR?

using ImageFiltering: AbstractBorder

padarray_fix(img::AbstractArray, border::AbstractBorder) = padarray_fix(eltype(img), img, border)
function padarray_fix(::Type{T}, img::AbstractArray, border) where {T}
    ba = BorderArray(img, border)
    out = similar(ba, T, axes(ba))
    out .= ba
end
mkitti commented 1 year ago

If you could write out a few simple test cases, that would be great.

andrew-saydjari commented 1 year ago

Ok, will do. I just wasn't sure if there was intended behavioral differences between copy! and .=, happy to try to do a few test cases in few days.

mkitti commented 1 year ago

I'm not sure if I fully understand the issue yet either, but the test cases would help confirm that we have a fix is in place and it stays fixed.