Make Plan class as a part of API

kokkos / kokkos-fft

A shared-memory FFT for the Kokkos ecosystem

https://kokkosfft.readthedocs.io/

Other

24 stars 5 forks source link

Make Plan class as a part of API #165

Closed yasahi-hpc closed 2 weeks ago

yasahi-hpc commented 3 weeks ago

Resolves #156

This PR aims at making Plan class as a part of API.

[x] KokkosFFT::Impl::Plan -> KokkosFFT::Plan
[x] KokkosFFT::Impl::fft_exec_impl -> ~~KokkosFFT::execute~~ plan.execute
[x] Remove unused variables from plan class m_in_T and m_out_T
[x] Update implementation details and tests
[x] Update example 6
[x] Update docs
[x] Make internal types in Plan class private

tpadioleau commented 3 weeks ago

I have questions about the API of Plan

do you think all member functions of KokkosFFT::Plan need to be public ?
are all member variables needed ? I don't think we use m_out_T ?
will the member functions appear in the documentation ?

yasahi-hpc commented 3 weeks ago

I have questions about the API of Plan

do you think all member functions of KokkosFFT::Plan need to be public ?

Actually, it may be better to make KokkosFFT::execute as a method of KokkosFFT::Plan class. This way we can make most functions private. I do not remember why I did not do so.

are all member variables needed ? I don't think we use m_out_T ?

Sorry about that. These should be suppressed. We may add an option to use these variables to reuse the internal buffers in the future.

will the member functions appear in the documentation ?

I think we can make most of them private and add docstrings to public methods only.

yasahi-hpc commented 3 weeks ago

@tpadioleau May I have your review please? I think most of the issues are resolved

yasahi-hpc commented 3 weeks ago

I had a quick look. I think it is better now that we hide most of member functions of KokkosFFT::Plan.

My main concern on the current API is about in and out views. They are being passed both in the constructor and in execute. It looks overconstrained, it is not clear to me whether one can use different views in execute from views that were passed in the constructor. As far as I understand it is due to the FFTW API. Reading FFTW documentation, I understand we can use different arrays but with some constraints.

@cedricchevalier19 any opinion on that ?

Thanks! execute interface is needed in order to reuse FFT plans. Following FFTW, we allow users to change the internal data of views, but do not allow to change

Types of views
extents of views

yasahi-hpc commented 2 weeks ago

@tpadioleau Thank you for reviews. If you feel good, I will merge this