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

Cleanup #8

Merged
merged 9 commits into from
Dec 14, 2023
Merged

Cleanup #8

merged 9 commits into from
Dec 14, 2023

Conversation

yasahi-hpc
Copy link
Collaborator

@yasahi-hpc yasahi-hpc commented Dec 11, 2023

Following modifications have been made.

  1. Use templates for most of the tests to reduce lines
  2. Some interfaces for FFT are suppressed which are more or less the same
  3. Avoid normalization if unnecessary

The remaining issues before release are as follows:

  • Unuse double underscore identifiers like __KOKKOSFFT_NORMALIZATION_HPP__
  • Rename FFT_Normalization to Normalization because they are already under KokkosFFT namespace
  • Support transforms over odd number. The number of points for FFT direction is limited to be even number (so that the size after transform would be n/2+1)
  • More static assertions. Particularly, we assume that the In and Out Views have same Layout and ranks, which should be checked.
  • Introduce ExecSpace as a template argument as well as DDC. This allows the coexistence of fft helpers for both host and device:
KokkosFFT::fft<Kokkos::OpenMP>(a, out); // a and out are on Host or Device is CPU
KokkosFFT::fft<Kokkos::Cuda>(a, out); // a and out are on Device

KokkosFFT::fft(Kokkos::OpenMP, a, out); // a and out are on Host or Device is CPU
KokkosFFT::fft(Kokkos::Cuda, a, out); // a and out are on Device

We can also make a check for memory and exec space consistency by

static_assert(
  Kokkos::SpaceAccessibility<ExecSpace, MemorySpace>::accessible,
  "MemorySpace has to be accessible for ExecutionSpace."
);
  • Add capability to reuse plans. This is particularly important for NVIDIA GPUs, which has some overheads to create plans.
  • Management of default arguments. As well as numpy, I would like to allow the function calls with minimum overloading
KokkosFFT::fft(a, out);
KokkosFFT::fft(a, out, /*axis=*/-1);
KokkosFFT::fft(a, out, /*n=*/n0);
KokkosFFT::fft(a, out, KokkosFFT::FFT_Normalization::BACKWARD);
KokkosFFT::fft(a, out, KokkosFFT::FFT_Normalization::BACKWARD, /*axis=*/*-1);
KokkosFFT::fft2(a, out, KokkosFFT::FFT_Normalization::BACKWARD, /*axes=*/axes_type{-2, -1});
KokkosFFT::fft(a, out, plan, KokkosFFT::FFT_Normalization::BACKWARD, /*axis=*/-1);

Particularly, it is difficult to distinguish n and axis, where n is a size_t type and axis is an int type.

  • Introduce the Impl namespace to disallow users to access implementation details
  • Add missing functions such as hfft
  • Add capability to allow optimal argument n. (See fft)
  • Googletest should be parameterized over types using ::testing::Types
  • Add docs to explain what are our interfaces and how to use them
  • introduce a clang format file ideally from Kokkos repo
  • Adding perf_test with googlebenchmark.
  • Improve docker files and add nvidia-hpc-sdk env

The remaining issues which may be important in the future

  • Allow to access from Views with strided Layouts. Internally, the input and/or output buffers with LayoutRight are created and the strided Views are copied into these buffers.
  • Add Intel GPU support based on oneMKL
  • Adding 4D to 8D capabilities
  • Seek for non-contiguous fat plans which may be available in some cases
  • Add more advanced examples. Derivatives in Fourier space, simulations with spectral method?

common/src/KokkosFFT_normalization.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_normalization.hpp Outdated Show resolved Hide resolved
This was referenced Dec 12, 2023
@yasahi-hpc yasahi-hpc mentioned this pull request Dec 13, 2023
31 tasks
@yasahi-hpc
Copy link
Collaborator Author

@jbigot OK to merge? Others are supposed to be resolved based on issues

@jbigot
Copy link
Member

jbigot commented Dec 14, 2023

please merge, it improves the code-base a lot

@yasahi-hpc yasahi-hpc merged commit 2a25891 into main Dec 14, 2023
2 checks passed
@yasahi-hpc yasahi-hpc deleted the cleanup branch December 14, 2023 15:55
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