Cleanup over empty #10
Conversation
…rom exec interface
| @@ -0,0 +1,28 @@ | |||
| cmake_minimum_required(VERSION 3.23) | |||
| project(kokkos-fft LANGUAGES CXX) | |||
There was a problem hiding this comment.
| project(kokkos-fft LANGUAGES CXX) | |
| project (KokkosFFT LANGUAGES CXX) |
Should be a good idea to add a version number.
| find_package(Kokkos CONFIG) | ||
| if(NOT kokkos_FOUND) | ||
| add_subdirectory(tpls/kokkos) |
There was a problem hiding this comment.
| find_package(Kokkos CONFIG) | |
| if(NOT kokkos_FOUND) | |
| add_subdirectory(tpls/kokkos) | |
| find_package (Kokkos) | |
| if (NOT Kokkos_FOUND) | |
| add_subdirectory (tpls/kokkos) |
Ideally, we should have a flag to turn off in source Kokkos to help packaging. It is important to fail if Kokkos is not found in the case of Spack.
There was a problem hiding this comment.
Sorry, this point is not very clear to me. I would appreciate if you could show me examples
| endif() | ||
|
|
||
| # Googletest | ||
| include(CTest) |
There was a problem hiding this comment.
| include(CTest) |
We do not need CTest here.
There was a problem hiding this comment.
In the end, we need this for execute testing of examples in actions?
|
|
||
| # Googletest | ||
| include(CTest) | ||
| if(BUILD_TESTING) |
There was a problem hiding this comment.
Option is not previously defined.
There was a problem hiding this comment.
This options would be needed for production, build with -DBUILD_TESTING=OFF
| find_package(GTest CONFIG) | ||
| if(NOT GTest_FOUND) | ||
| add_subdirectory(tpls/googletest) |
There was a problem hiding this comment.
Same comment as for Kokkos
| using array_layout_type = typename ViewType::array_layout; | ||
|
|
||
| // Convert the input axes to be in the range of [0, rank-1] | ||
| std::vector<int> axes; |
There was a problem hiding this comment.
Why a vector for something of fixed dimensions ?
There was a problem hiding this comment.
I would like to test a vector in this line
| ) | ||
|
|
||
| target_compile_features(unit-tests-kokkos-fft-common PUBLIC cxx_std_17) | ||
| target_compile_options(unit-tests-kokkos-fft-common PUBLIC -std=c++17) |
There was a problem hiding this comment.
Why ?? we should not hardcode this kind of flags.
There was a problem hiding this comment.
The bottom one can be suppressed? The point is that Kokkos cannot be built without C++17 capability, so I would like to enforce that
| double rtol=1.e-5, | ||
| double atol=1.e-8 | ||
| ) { |
There was a problem hiding this comment.
We need some fixed numbers anyway. They come from numpy
| Kokkos::kokkos | ||
| ) | ||
|
|
||
| target_include_directories(fft INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}) |
There was a problem hiding this comment.
Does not work when installed!
There was a problem hiding this comment.
I will add install capability
| ) | ||
|
|
||
| target_include_directories(fft INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}) | ||
| add_library(Kokkos::fft ALIAS fft) |
|
I put most of the comments in #11 (comment). I am willing to close this PR and make another PR to resole tasks in #11 (comment). Is that OK for you? @jbigot @cedricchevalier19 |
|
As you wish @yasahi-hpc |
|
Most of the implementations are made based on the comments. I will close this. |
This is a fake PR for #8 to include the whole repo