search

bionanoimaging / FourierTools.jl

Tools for working with Fourier space.
https://bionanoimaging.github.io/FourierTools.jl/stable/
MIT License
54 stars 6 forks source link

Remove wrong rule to fix issue #35 #36

Closed roflmaostc closed 9 months ago

roflmaostc commented 1 year ago

Hi @trahflow,

AD support should be ok, now?

roflmaostc commented 1 year ago

Tests are still passing: https://github.com/bionanoimaging/FourierTools.jl/blob/8fbc1cafb2e19c4687154490b5bb93c21cbbf56b/test/convolutions.jl#L100

trahflow commented 1 year ago

Thanks for the fix!

I think the rrule for the planned version is still incorrect. However the adjoints for planned real ffts in Zygote are also incorrect, so just removing the rule won't fix everything (until JuliaMath/AbstractFFTs.jl#67 is released)

roflmaostc commented 1 year ago

Ah yes, referring to this: https://github.com/bionanoimaging/FourierTools.jl/blob/8fbc1cafb2e19c4687154490b5bb93c21cbbf56b/src/convolutions.jl#L222

codecov[bot] commented 1 year ago

Codecov Report

Merging #36 (79cd82a) into main (87352d2) will decrease coverage by 0.35%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
- Coverage   93.34%   93.00%   -0.35%     
==========================================
  Files          17       17              
  Lines         947      958      +11     
==========================================
+ Hits          884      891       +7     
- Misses         63       67       +4
Impacted Files Coverage Δ
src/convolutions.jl 97.91% <ø> (-0.45%) :arrow_down:

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.