JuliaImages / ImageQualityIndexes.jl

Indexes for image quality assessment
MIT License
9 stars 10 forks source link

remove `ImageFiltering` dependency #10

Closed johnnychen94 closed 3 years ago

johnnychen94 commented 5 years ago

There're two reasons for this:

LakshyaKhatri commented 4 years ago

Hello @johnnychen94, I want to work on this issue. If you could help me with some pointers, maybe I can implement this :smile: . Also, I didn't find anything regarding Zygote.

johnnychen94 commented 4 years ago

Thanks for the interest @LakshyaKhatri

I haven't done enough research yet, but basically what's needed to be implemented is a simplified version of imfilter.

Given that Zygote is FFTW compatible, we could provide a fftw-based convolution operation with the famous convolution theorem here. Once that's done, we could then replace the usage of imfilter and then get rid of the big Imagefiltering dependency.

Some of the codes might need to be modified accordingly to be differentiable, though. However, note that in other implementations(e.g., https://github.com/Po-Hsun-Su/pytorch-ssim) , usually the SSIM is simplified and optimized for 4D Tensor.

johnnychen94 commented 4 years ago

Making JuliaImages zygote-friendly is a giant project. TBH, I don't know yet what Zygote-compatible means to JuliaImages. Since Images are typically represented as Array{<:Colorant} in JuliaImages world, things can be completely different from how we process images (4D numeric tensor) in pytorch/tensorflow.

I'm wondering if I can get some advice from @Avik-Pal and @timholy about this.

timholy commented 4 years ago

Perhaps the most important step is allowing dual numbers as the eltype of colortypes. Then autodiff will Just Work, right?

avik-pal commented 4 years ago

I think allowing dual numbers to work will allow only ForwardDiff / ReverseDiff to work. Zygote in theory (Here is a small example using Colors.jl.) should work for Array{<:Colorant} but we might need to add some custom adjoints to make the process smooth.

Making the entirety of JuliaImages Zygote friendly definitely is a big project but, making the gradients work for SSIM should not be very difficult. I can see 2 possible solutions:

  1. Since all of the operations are differentiable and the only issue might occur at imfilter so a) add a custom adjoint for it (definitely non-trivial) or b) rewrite of imfilter with FFTW and things will work out of the box.
  2. Instead of imfilter use NNlib convolutions (which work really well on GPUs and for batched data) as @johnnychen94 suggested. In this case, adjoints are already defined. Though I feel this solution is more appropriate to have inside Flux itself as a loss function rather than here as it uses 4D Arrays instead of the default julia images types.
johnnychen94 commented 3 years ago

I think removing ImageFiltering dependency might be impractical if we ever want to extend this library. Thus I'm closing this for now.

The effort should be put into making imfilter Zygote-friendly; there's an initial WIP PR for this in https://github.com/SomTambe/DiffImages.jl/pull/21.