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

Apply to_array helper #171

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yasahi-hpc
Copy link
Collaborator

Improves #170

This PR aims at introducing to_array helper which is used in Transpose functions.
For the compile time testing, we need a newer version of Kokkos.

@yasahi-hpc yasahi-hpc self-assigned this Oct 12, 2024
@yasahi-hpc yasahi-hpc added enhancement New feature or request cleanup labels Oct 12, 2024
@yasahi-hpc yasahi-hpc marked this pull request as draft October 12, 2024 08:43
Remove unnecessary variadic integers

Co-authored-by: Damien L-G <dalg24+github@gmail.com>
@yasahi-hpc
Copy link
Collaborator Author

We may need a newer version of Kokkos for nicer tests. Should we update Kokkos to 4.4 and update docs accordingly?

Copy link
Collaborator

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

We can try

common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
common/unit_test/Test_Utils.cpp Outdated Show resolved Hide resolved
@yasahi-hpc yasahi-hpc marked this pull request as ready for review October 14, 2024 09:06
Copy link
Collaborator

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +560 to +563
// [TO DO] Need to update Kokkos version for compile time test
// GTEST_SKIP() << "Skipping ToArray::rvalue compile time test";
// static_assert(KokkosFFT::Impl::to_array(std::array{1, 2}) ==
// Kokkos::Array{1, 2});
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want, you can already implement the test using the comparison operator inside ifdef based on the version of Kokkos ?

Copy link
Collaborator Author

@yasahi-hpc yasahi-hpc Oct 14, 2024

Choose a reason for hiding this comment

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

If you are fine, I would like to keep it in comments since we do not test that in CI for the moment. Though, I am pretty sure it works at least with Kokkos v.4.4.1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I am ok with that. One question though, how do we keep track that we will need to update this snippet when we increase the minimum version of Kokkos ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about tracking in an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not really confident on remembering that there is actually an issue about this when we bump the minimum version. We can try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants