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

In-place real-to-complex FFTs #65

Merged
merged 22 commits into from
May 25, 2023
Merged

Conversation

fbignonnet
Copy link
Contributor

This PR is an attempt to address the issue #21, based on FFTW.jl

The following modifications are made:

  1. Definition of RFFT! and BRFFT! transforms in Transforms/r2c.jl
  2. Definition of functions plan_rfft! and plan_brfft! extending FFTW.jl for the creation of plans for in-place rfft! and brfft! in Transforms/r2c.jl
  3. Definition of a ManyPencilArrayRFFT! container in multiarrays_r2c.jl container which adapts ManyPencilArray. In this part, a real data vector must reinterpreted as a complex vector. This is done using convert of a pointer from Float to Complex and unsafe_wrap, instead of using the Julia reinterpret function, as suggested in this FFTW pull-request. In future, it might be more appropriate to redefine ManyPencilArray or create a supertype to ManyPencilArrayRFFT! and ManyPencilArray directly in PencilArrays.jl to avoid multiple method redefinitions. For now I didn't want to make separate PRs for testing purposes.
  4. A ManyPencilArrayRFFT! is allocated by a PencilFFTPlan whenever appropriate in allocate.jl.
  5. Adaptation of operations.jl to handle RFFT! forward and backward. In the mean time, the final scaling of the backward operations is separated from the actual backward transforms (see e.g. the difference between an ifft and a bfft in AbstractFFTs.jl), and a new function bmul! is provided in addition to the existing mul! and ldiv!.
  6. Creation of new test sets in test/rfft.jl to test RFFT! against RFFT

@jipolanco
Copy link
Owner

Looks great, thanks for this very nice work.

I agree that it would be better to modify the definition of ManyPencilArray in PencilArrays and to avoid the definition of a ManyPencilArrayRFFT! type. But I guess it can wait until later, once we're sure that the current implementation works.

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #65 (c22f1df) into master (309eec5) will decrease coverage by 1.16%.
The diff coverage is 92.37%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
- Coverage   98.13%   96.97%   -1.16%     
==========================================
  Files          10       11       +1     
  Lines         429      529     +100     
==========================================
+ Hits          421      513      +92     
- Misses          8       16       +8     
Impacted Files Coverage Δ
src/PencilFFTs.jl 100.00% <ø> (ø)
src/Transforms/r2c.jl 88.70% <86.04%> (-4.15%) ⬇️
src/operations.jl 97.82% <90.62%> (-2.18%) ⬇️
src/allocate.jl 100.00% <100.00%> (ø)
src/multiarrays_r2c.jl 100.00% <100.00%> (ø)

@jipolanco
Copy link
Owner

Thanks again for this very nice addition. I took the liberty of doing some minor modifications, in particular simplifying the logic of allocating arrays for in-place transforms.

As for the code duplication between ManyPencilArray and ManyPencilArrayRFFT!, I just tried to implement your suggestion of creating a common parent type AbstractManyPencilArray (which I think it's the easiest and best solution to the problem), see jipolanco/PencilArrays.jl#81. After that PR is merged, we can define:

    ManyPencilArrayRFFT!{T,N,M} <: AbstractManyPencilArray{N,M}

and remove most of the methods currently defined for ManyPencilArrayRFFT!.

@fbignonnet
Copy link
Contributor Author

Great ! Things will look much cleaner with your modifications. They should also ease the possibility to allocate data for a BRFFT!, which wasn't covered in my commits. Test sets might need to be broadened for RFFT! and BRFFT!

Many thanks to you for these nice libraries that ease so much the use of MPI for arrays and FFTs.

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

Successfully merging this pull request may close these issues.

2 participants