Closed league closed 2 years ago
Lol that configure update was a quick response, are you a bot? Add 34957 to 70764.
My understanding of this problem is that c2r transforms in CUDA 10.1+ are allowed to change the input array. That causes problems for Bifrost when fftshift=True
since the transform rearranges the input while Bifrost is trying to rearrange the input. It would be great if we could actually fix this problem. "Fix" in this case could be either:
1) we don't support c2r with fftshift=True
or
2) we catch this case and pre-shift the inputs before calling the transform.
I would prefer (2) but I'd have to think about how to do that. Only changing the bifrost.fft
module leaves people in C/C++ out of luck. Changing fft.cu
is a better option but more involved. Before starting down either road we should verify that fftshift=True
is the only problem and that it isn't a general c2r thing.
Merging #181 (8b88dbb) into master (eb5a8c5) will not change coverage. The diff coverage is
n/a
.:exclamation: Current head 8b88dbb differs from pull request most recent head be639c0. Consider uploading reports for the commit be639c0 to get more accurate results
@@ Coverage Diff @@
## master #181 +/- ##
=======================================
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 eb5a8c5...be639c0. Read the comment docs.
I've started a new fft-c2r-fix
branch to fix the underlying issue. My goal is to catch c2r transforms on CUDA 10.1+ and shift the input data before the cuFFT call rather than relying on the pre_fftshift
function in the callback during the cuFFT call.
“Fixes” is a strong word, but I suppose this can close #180. I'm just updating the warnings and exclusion of the c2r tests for CUDA 11.7, which were reported therein. @jaycedowell, I guess it would need an autoconf rerun before merge.