Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove ImageFiltering dependency #10

Closed
johnnychen94 opened this issue Sep 16, 2019 · 6 comments
Closed

remove ImageFiltering dependency #10

johnnychen94 opened this issue Sep 16, 2019 · 6 comments

Comments

@johnnychen94
Copy link
Member

johnnychen94 commented Sep 16, 2019

There're two reasons for this:

  • ImageFiltering is heavy; currently the only place it's used is imfilter in SSIM.
  • To make SSIM compatible to Zygote, we might need to use a simplified version of imfilter built only upon FFTW.
@LakshyaKhatri
Copy link

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

@johnnychen94
Copy link
Member Author

johnnychen94 commented Jun 25, 2020

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
Copy link
Member Author

johnnychen94 commented Jun 25, 2020

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
Copy link
Member

timholy commented Jun 25, 2020

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

@avik-pal
Copy link

avik-pal commented Jun 26, 2020

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
Copy link
Member Author

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 FluxML/DiffImages.jl#21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants