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

Minor improvement of README #27

Closed
wants to merge 2 commits into from
Closed

Minor improvement of README #27

wants to merge 2 commits into from

Conversation

yasahi-hpc
Copy link
Collaborator

  1. Improve Usage section
  2. Add disclaimer that we may change the code

@yasahi-hpc
Copy link
Collaborator Author

Hi @cedricchevalier19, may I have your review on README? Your comments are highly appreciated as usual.

@@ -6,8 +6,8 @@ UNOFFICIAL FFT interfaces for Kokkos C++ Performance Portability Programming Eco

KokkosFFT implements local interfaces Kokkos and de facto standard FFT libraries, including fftw, cufft and hipfft.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can add a link to these FFT libraries?

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) interfaces based on [Kokkos](https://github.com/kokkos/kokkos).
A key concept is that "As easy as numpy, as fast as vendor libraries". Accordingly, our APIs follow the APIs by [numpy.fft](https://numpy.org/doc/stable/reference/routines.fft.html) with minor differences. If something is wrong with runtime values, it will raise runtime errors. 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.

s/APIs/API/g

it will raise runtime errors.

C++ exceptions? Be more precise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have two errors: std::runtime_error and assert. Since this is README, I would like to make it more precise in docs, which is under development. Or do you think it is better to be precise in README?

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

There are two major differences: ```execution_space``` argument and updating output as argument not returned. 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.

updating output as argument not returned

Not very clear

Also try to define/link to execution_space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add a link to execution_space. How about rephrasing as
"output value is an argument of APIs (not returned from APIs)."

Comment on lines +115 to +116
Is is assumed that the Kokkos is installed under ```<lib_dir>/kokkos``` targeting Skylake 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.

Why Skylake ? I do not see anything specific with this architecture.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simply because I am using Icelake in my environment. Any CPU is fine, just an example.

@@ -6,8 +6,8 @@ UNOFFICIAL FFT interfaces for Kokkos C++ Performance Portability Programming Eco

KokkosFFT implements local interfaces Kokkos and de facto standard FFT libraries, including fftw, cufft and hipfft.
"Local" means not using MPI, or running within a
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) interfaces based on [Kokkos](https://github.com/kokkos/kokkos).
Copy link
Member

Choose a reason for hiding this comment

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

I do not think that we implement the numpy interface. numpy-like interface (adapted for Kokkos) seems more accurate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. I will modify the sentence accordingly

@yasahi-hpc
Copy link
Collaborator Author

@cedricchevalier19 Thanks a lot for your review!

@yasahi-hpc yasahi-hpc mentioned this pull request Jan 25, 2024
@yasahi-hpc
Copy link
Collaborator Author

Closing. Modifications are made in PR (#36)

@yasahi-hpc yasahi-hpc closed this Jan 26, 2024
@yasahi-hpc yasahi-hpc deleted the update-readme branch January 26, 2024 08:17
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