JuliaImages / ImageFiltering.jl

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

Probably severe argument dropping #112

Open rapus95 opened 5 years ago

rapus95 commented 5 years ago

https://github.com/JuliaImages/ImageFiltering.jl/blob/248cfde56d7e8b2df2c59de26a0ef41e1dafe124/src/imfilter.jl#L588-L590 There the args... is just dropped silently. I guess that is a copy & paste mistake and the args... should be omitted here.

Edit: In case that's the correct observation, there's a pull request: https://github.com/JuliaImages/ImageFiltering.jl/pull/113

johnnychen94 commented 5 years ago

If I understand it right, for this method, the only valid option left in args... is alg; I think the right dispatch should be

function imfilter!(out::AbstractArray, img::AbstractArray, kernel::ProcessedKernel, args...) 
    imfilter!(out, img, kernel, Pad(:replicate), args...) 
end

FYI, For method in the next line, we drop args... because we need to avoid specifying both AbstractResource and Algorithm

https://github.com/JuliaImages/ImageFiltering.jl/blob/248cfde56d7e8b2df2c59de26a0ef41e1dafe124/src/imfilter.jl#L592-L594

rapus95 commented 5 years ago

If I understand it right, for this method, the only valid option left in args... is alg; I think the right dispatch should be

function imfilter!(out::AbstractArray, img::AbstractArray, kernel::ProcessedKernel, args...) 
    imfilter!(out, img, kernel, Pad(:replicate), args...) 
end

Which method is called if there was a pad argument in the args...? and on the other hand, why don't we just write it out then? -> remove args... and place alg::AbstractAlgorithm(or similar). Silently dropping should never occur I guess.

FYI, For method in the next line, we drop args... because we need to avoid specifying both AbstractResource and Algorithm

https://github.com/JuliaImages/ImageFiltering.jl/blob/248cfde56d7e8b2df2c59de26a0ef41e1dafe124/src/imfilter.jl#L592-L594 That one I saw and understood 😄

johnnychen94 commented 5 years ago

Which method is called if there was a pad argument in the args...?

It finds the one with AbstractBorder or AbstractString annotation:

https://github.com/JuliaImages/ImageFiltering.jl/blob/248cfde56d7e8b2df2c59de26a0ef41e1dafe124/src/imfilter.jl#L596-L609

and on the other hand, why don't we just write it out then?

I think what we need to do is to review the whole dispatching rules and types provided by ImageFiltering, and see if we can get a simplified one that meets all the needs. Making small patches to the current rules doesn't make much difference.

timholy commented 5 years ago

Yes, the dispatch is very complex, and took me a very long time to "get right" (to the extent that it's right). Now that Julia has fast keyword arguments, should we just make everything a keyword argument?