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

Update readme #36

Merged
merged 11 commits into from
Jan 26, 2024
Merged

Update readme #36

merged 11 commits into from
Jan 26, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

Cherry-pick from the previous PR (#27) with modifications based on comments. Because of the breaking changes in PR (#30), I made a new PR. The README is planed to be further updated before release.

@yasahi-hpc
Copy link
Collaborator Author

Hi @cedricchevalier19 , because of the breaking change in (#30), I made a new PR. If this is good for you, I will merge this.

@yasahi-hpc yasahi-hpc added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 25, 2024
@yasahi-hpc yasahi-hpc self-assigned this Jan 25, 2024
Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one big issue is left out: how user have to specify which fft libraries to use?

This should be in the CMake examples. You should try to copy and paste the lines to be sure that they work at least on one machine.

I am not sure by all the markdown syntax, specially where blank lines are needed, so please use a markdown linter.

README.md Outdated
single MPI process without knowing about MPI. We are inclined to implement the numpy FFT interfaces based on Kokkos.
Here is the example for 1D case in python and KokkosFFT.
single MPI process without knowing about MPI. We are inclined to implement the [numpy.fft](https://numpy.org/doc/stable/reference/routines.fft.html)-like interfaces adapted for [Kokkos](https://github.com/kokkos/kokkos).
A key concept is that "As easy as numpy, as fast as vendor libraries". Accordingly, our API follows the API by [numpy.fft](https://numpy.org/doc/stable/reference/routines.fft.html) with minor differences. If something is wrong with runtime values (say ```View``` extents), it will raise runtime errors (C++ exceptions or assertions). Here is the example for 1D real to complex transform with ```rfft``` in python and KokkosFFT.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A key concept is that "As easy as numpy, as fast as vendor libraries". Accordingly, our API follows the API by [numpy.fft](https://numpy.org/doc/stable/reference/routines.fft.html) with minor differences. If something is wrong with runtime values (say ```View``` extents), it will raise runtime errors (C++ exceptions or assertions). Here is the example for 1D real to complex transform with ```rfft``` in python and KokkosFFT.
A key concept is that "As easy as numpy, as fast as vendor libraries". Accordingly, our API follows the API by [numpy.fft](https://numpy.org/doc/stable/reference/routines.fft.html) with minor differences. If something is wrong with runtime values (say `View` extents), it will raise runtime errors (C++ exceptions or assertions). Here is the example for 1D real to complex transform with `rfft` in python and KokkosFFT.

Concerning assertions, I know it is done this way in some part of Kokkos, however I think it's a bad behaviour for a library. It should not interrupt the calling application, where a proper error handling can be done. Not something to correct now, but perhaps something to keep in mind for the Kokkos ecosystem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Let us discuss this point in our next meeting

README.md Outdated
@@ -33,6 +33,8 @@ Kokkos::fence();
KokkosFFT::rfft(execution_space(), x, x_hat);
```

There are two major differences: [```execution_space```](https://kokkos.org/kokkos-core-wiki/API/core/execution_spaces.html) argument and output value (```x_hat```) is an argument of API (not returned from API). As imagined, KokkosFFT only accepts [Kokkos Views](https://kokkos.org/kokkos-core-wiki/API/core/View.html) as input data. The accessibilities of Views from ```execution_space``` is statically checked (compilation errors if not accessible).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
There are two major differences: [```execution_space```](https://kokkos.org/kokkos-core-wiki/API/core/execution_spaces.html) argument and output value (```x_hat```) is an argument of API (not returned from API). As imagined, KokkosFFT only accepts [Kokkos Views](https://kokkos.org/kokkos-core-wiki/API/core/View.html) as input data. The accessibilities of Views from ```execution_space``` is statically checked (compilation errors if not accessible).
There are two major differences: [`execution_space`](https://kokkos.org/kokkos-core-wiki/API/core/execution_spaces.html) argument and output value (`x_hat`) is an argument of API (not returned from API). As imagined, KokkosFFT only accepts [Kokkos Views](https://kokkos.org/kokkos-core-wiki/API/core/View.html) as input data. The accessibilities of Views from `execution_space` is statically checked (compilation errors if not accessible).

README.md Outdated
@@ -60,7 +62,106 @@ int axis = -1;
KokkosFFT::rfft(execution_space(), x, x_hat, KokkosFFT::FFT_Normalization::BACKWARD, axis); // FFT along -1 axis and batched along 0th axis
```

## Building KokkosFFT
In this example, the 1D batched ```rfft``` over 2D View along ```axis -1``` is executed. Some basic examples are found in [examples](https://github.com/CExA-project/kokkos-fft/tree/main/examples).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In this example, the 1D batched ```rfft``` over 2D View along ```axis -1``` is executed. Some basic examples are found in [examples](https://github.com/CExA-project/kokkos-fft/tree/main/examples).
In this example, the 1D batched `rfft` over 2D View along `axis -1` is executed. Some basic examples are found in [examples](https://github.com/CExA-project/kokkos-fft/tree/main/examples).

README.md Outdated
In this example, the 1D batched ```rfft``` over 2D View along ```axis -1``` is executed. Some basic examples are found in [examples](https://github.com/CExA-project/kokkos-fft/tree/main/examples).

## Disclaimer
KokkosFFT is under development and subject to change without warning. The authors do not guarantee that this code runs correctly in all the environments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
KokkosFFT is under development and subject to change without warning. The authors do not guarantee that this code runs correctly in all the environments.
** KokkosFFT is under development and subject to change without warning. The authors do not guarantee that this code runs correctly in all the environments. **

README.md Outdated
Comment on lines 77 to 78
Since KokkosFFT is a header-only library, it is enough to simply add as a subdirectory. It is assumed that kokkos and kokkosFFT are placed under ```<project_directory>/tpls```.
Here is an example to use KokkosFFT in the following CMake project.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Since KokkosFFT is a header-only library, it is enough to simply add as a subdirectory. It is assumed that kokkos and kokkosFFT are placed under ```<project_directory>/tpls```.
Here is an example to use KokkosFFT in the following CMake project.
Since KokkosFFT is a header-only library, it is enough to simply add as a subdirectory. It is assumed that kokkos and kokkosFFT are placed under `<project_directory>/tpls`.
Here is an example to use KokkosFFT in the following CMake project.

README.md Outdated
This way, all the functionalities are executed on A100 GPUs.

### Install as a library
Is is assumed that the Kokkos is installed under ```<lib_dir>/kokkos``` targeting Icelake with OpenMP backend. ```Kokkos_dir``` also needs to be set appropriately. Here is a recipe to install KokkosFFT under ```<lib_dir>/kokkosFFT```.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Is is assumed that the Kokkos is installed under ```<lib_dir>/kokkos``` targeting Icelake with OpenMP backend. ```Kokkos_dir``` also needs to be set appropriately. Here is a recipe to install KokkosFFT under ```<lib_dir>/kokkosFFT```.
Is is assumed that the Kokkos is installed under `<install_dir>/kokkos` with OpenMP backend. Here is a recipe to install KokkosFFT under `<install_dir>/kokkos_fft`.

README.md Outdated
Comment on lines 118 to 128
export KOKKOSFFT_INSTALL_PREFIX=<lib_dir>/kokkosFFT
export KokkosFFT_DIR=<lib_dir>/kokkosFFT/lib64/cmake/kokkos-fft

mkdir build_KokkosFFT && cd build_KokkosFFT
cmake -DBUILD_TESTING=OFF \
-DCMAKE_CXX_COMPILER=icpx \
-DKokkos_ENABLE_OPENMP=ON \
-DKokkos_ARCH_SKX=ON \
-DCMAKE_INSTALL_PREFIX=${KOKKOSFFT_INSTALL_PREFIX} ..
cmake --build . -j 8
cmake --install .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export KOKKOSFFT_INSTALL_PREFIX=<lib_dir>/kokkosFFT
export KokkosFFT_DIR=<lib_dir>/kokkosFFT/lib64/cmake/kokkos-fft
mkdir build_KokkosFFT && cd build_KokkosFFT
cmake -DBUILD_TESTING=OFF \
-DCMAKE_CXX_COMPILER=icpx \
-DKokkos_ENABLE_OPENMP=ON \
-DKokkos_ARCH_SKX=ON \
-DCMAKE_INSTALL_PREFIX=${KOKKOSFFT_INSTALL_PREFIX} ..
cmake --build . -j 8
cmake --install .
cmake -b build_kokkos_fft -DBUILD_TESTING=OFF -DKokkos_ROOT="<install_dir>/kokkos" -DCMAKE_INSTALL_PREFIX="<install_dir>/kokkos_fft" .
cmake --build build_kokkos_fft --parallel && cmake --install build_kokkos_fft

README.md Outdated
└──hello.cpp
```

The ```CMakeLists.txt``` would be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The ```CMakeLists.txt``` would be
The `CMakeLists.txt` would be

README.md Outdated
The ```CMakeLists.txt``` would be
```CMake
cmake_minimum_required(VERSION 3.23)
project(kokkos-fft LANGUAGES CXX)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a good project name

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I will change it

README.md Outdated
Comment on lines 154 to 162
export KOKKOSFFT_INSTALL_PREFIX=<lib_dir>/kokkosFFT
export KokkosFFT_DIR=<lib_dir>/kokkosFFT/lib64/cmake/kokkos-fft

mkdir build && cd build
cmake -DCMAKE_CXX_COMPILER=icpx \
-DCMAKE_BUILD_TYPE=Release \
-DKokkos_ENABLE_OPENMP=ON \
-DKokkos_ARCH_SKX=ON ..
cmake --build . -j 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export KOKKOSFFT_INSTALL_PREFIX=<lib_dir>/kokkosFFT
export KokkosFFT_DIR=<lib_dir>/kokkosFFT/lib64/cmake/kokkos-fft
mkdir build && cd build
cmake -DCMAKE_CXX_COMPILER=icpx \
-DCMAKE_BUILD_TYPE=Release \
-DKokkos_ENABLE_OPENMP=ON \
-DKokkos_ARCH_SKX=ON ..
cmake --build . -j 8
cmake -b build_dir -DCMAKE_PREFIX_PATH="<install_dir>/kokkos;<install_dir>/kokkos_fft" .
cmake --build build_dir --parallel

I do not understand why Kokkos_ENABLE_OPENMP was passed here if Kokkos is already installed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true. May be unnecessary

@yasahi-hpc
Copy link
Collaborator Author

Thanks again for your review.

I think one big issue is left out: how user have to specify which fft libraries to use?

This should be in the CMake examples. You should try to copy and paste the lines to be sure that they work at least on one machine.

Actually, I am not planning to allow a user to specify fft libraries. A fft library dedicated to Kokkos Device backend is automatically chosen, e.g.,
cufft with -DKokkos_ENABLE_CUDA=ON
hipfft with -DKokkos_ENABLE_HIP=ON
oneMKL with -DKokkos_ENABLE_SYCL=ON
fftw with -DKokkos_ENABLE_<OPENMP, THREAD, SERIAL>=ON

If Kokkos Device backend is given, then only the library for device is available. If a user want to call fft from both host and device, it is needed to enable -DKokkosFFT_ENABLE_HOST_AND_DEVICE. With this option, the user can call fftw from host_execution_space on host Views. This point will be added in the prerelease stage.

I am not sure by all the markdown syntax, specially where blank lines are needed, so please use a markdown linter.

OK. I will try some plugin

@cedricchevalier19
Copy link
Member

I was talking on how to find or help finding the correct fft implementations.

However, adding a table and the corresponding library can be a great addition in the readme or the documentation.
Thanks for your clarifications.

@yasahi-hpc
Copy link
Collaborator Author

I was talking on how to find or help finding the correct fft implementations.

Do you mean the complete documentations of API? I agree it is the most important missing ingredient for this repo. I am currently working on that.

@yasahi-hpc
Copy link
Collaborator Author

Actually, I found an issue to build a code in case both Kokkos and KokkosFFT are added as subdirectories of a CMake project. Before find_package(Kokkos), I would test if Kokkos_PATH is set or not. I am not quite sure this is the best way to check whether Kokkos is already added as a subdirectory or not.

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know how standard is kokkos-path

CMakeLists.txt Outdated
Comment on lines 13 to 18
# First check, Kokkos is added as subdirectory or not
if (KOKKOS_PATH STREQUAL "")
find_package(Kokkos REQUIRED)
else()
message(STATUS "Using Kokkos at ${KOKKOS_PATH}")
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, don't do this.
Use can check for kokkos::kokkos target or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I modified. I will probably add a capability to display the Kokkos version detail in CMake in the future.

@cedricchevalier19
Copy link
Member

I was talking on how to find or help finding the correct fft implementations.

Do you mean the complete documentations of API? I agree it is the most important missing ingredient for this repo. I am currently working on that.

No, I mean how to find fftw and so if it is not straightforward.

@yasahi-hpc
Copy link
Collaborator Author

I was talking on how to find or help finding the correct fft implementations.

Do you mean the complete documentations of API? I agree it is the most important missing ingredient for this repo. I am currently working on that.

No, I mean how to find fftw and so if it is not straightforward.

As you said, finding fftw is not straightforward. I have prepared a helper to find it, but it would probably only work if fftw is installed in "conventional location". If a user manually install it, I am not sure what is the better way.

@yasahi-hpc
Copy link
Collaborator Author

@cedricchevalier19 Regarding the fftw documentations, I would like to make it in another PR. If you are OK, I would merge this

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@yasahi-hpc
Copy link
Collaborator Author

@cedricchevalier19 Thank you again for your review. I will merge this

@yasahi-hpc yasahi-hpc merged commit 1da85a9 into main Jan 26, 2024
5 checks passed
@yasahi-hpc yasahi-hpc deleted the update-readme-tmp branch January 26, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants