search

ledatelescope / bifrost

A stream processing framework for high-throughput applications.
BSD 3-Clause "New" or "Revised" License
64 stars 29 forks source link

complex-to-real FFT fix #182

Closed jaycedowell closed 1 year ago

jaycedowell commented 2 years ago

This PR addresses recent (CUDA 10.1+) problems with the FFT test suite. These problems turned out to be caused by two things:

  1. There was a change to cuFFT that caused complex-to-real transforms to overwrite the input array which interfered with the fftshift keyword, and
  2. There was an (apparent) change in the behavior in the undefined case of the first (or last) input value along the transpose axis having an imaginary component. The change looks to have moved the results away from what numpy.fft.irfft does. This seems to be more problematic on more recent GPUs.

Addressing (2) was a matter of updating the FFT test suite to always make a set of complex numbers that will work with a complex-to-real transform. This involved randomly generating real data and then running a real-to-complex transform to get the complex input data for the tests.

Fixing (1) was not so much fixing as realizing that applying a fftshift to data going into a complex-to-real transform doesn't make much sense. The fftshift makes since for complex-to-complex transforms where you end up with frequency bins that run from DC to Nyquist, -Nyquist to DC but you (or at least I) really want frequency bins from -Nyquist to Nyquist. You use the fftshift to put the data in a sensible frequency order for the forward transform and then undo that for you for the reverse transform. In the real-to-complex case there are no negative frequencies so there is nothing to want to shift. The fix is then to ignore the fftshift keyword and never shift if we are in the complex-to-real or real-to-complex cases. This is akin to what is done with the inverse keyword where it is ignored for real-to-complex and complex-to-real transforms since the data types set the direction.

codecov-commenter commented 2 years ago

Codecov Report

Merging #182 (28d788b) into master (be639c0) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #182   +/-   ##
=======================================
  Coverage   66.81%   66.81%           
=======================================
  Files          69       69           
  Lines        7410     7410           
=======================================
  Hits         4951     4951           
  Misses       2459     2459

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bba93a3...28d788b. Read the comment docs.

jaycedowell commented 2 years ago

@telegraphic do you agree with this reasoning?