ImageFiltering.jl icon indicating copy to clipboard operation
ImageFiltering.jl copied to clipboard

Probably severe argument dropping

Open rapus95 opened this issue 6 years ago • 4 comments
trafficstars

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

rapus95 avatar Aug 29 '19 23:08 rapus95

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

johnnychen94 avatar Aug 30 '19 06:08 johnnychen94

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 😄

rapus95 avatar Aug 31 '19 01:08 rapus95

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.

johnnychen94 avatar Aug 31 '19 08:08 johnnychen94

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?

timholy avatar Sep 04 '19 09:09 timholy