Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Global CMake for tutorials #91
base: main
Are you sure you want to change the base?
Global CMake for tutorials #91
Changes from 26 commits
1b2b56a
50bf4e0
56003c8
6268211
dec987e
4f30736
f2cc0c8
9ebf652
c4451a0
1a9d282
1b1dedc
1a07316
9bce058
7c6a676
8b5cd95
5779c0f
dacde70
2bc96df
46fa1d0
d6e6530
fe583b3
5656d32
af3141e
5ccfe61
7bdf80c
bfe7e16
798effa
a451ee4
0e178f0
90f978c
40db39d
96cf66d
dba7191
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not the latest release? Did you intend to update in a separate pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought a separate PR made more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be moved into a separate function instead of repeating it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like a
kokkos_tutorials_warn_cpu_only_program()
function or whatnot?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that each exercise can be used as a standalone. I think factorizing with a function or macro makes the example less clear, and I wish I could avoid including a separate (but shared) file at the terminal CMakeLists.txt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It some way it is akin to the weird find kokkos module isn't it? I'd slightly prefer reusing code as Jakob suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had trouble finding the proper solution as the
Kokkos_DEVICES
variable is not apparent. If I do a function, I will hide the details that might be useful to copy and paste for some users. That's the reason why it is better to be transparent there.There are only two such exercises; we will probably not have to touch the CMakeLists.txt too often, so the function is not worth it.
However, I can add a comment explaining what this conditional does for better logic readability (which is the function's main advantage for me).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here, please propose this change as its own PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was necessary in order to compile this exercise. I will comment out the
add_subdirectory
and do a separate PR.This file was deleted.