-
Notifications
You must be signed in to change notification settings - Fork 407
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
Change Enums in ScatterView to types #3076
Conversation
struct DefaultContribution; | ||
|
||
#ifdef KOKKOS_ENABLE_SERIAL | ||
template <> | ||
struct DefaultDuplication<Kokkos::Serial> { | ||
enum : int { value = Kokkos::Experimental::ScatterNonDuplicated }; | ||
using duplication_type = Kokkos::Experimental::ScatterNonDuplicated; |
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.
"tag" rather than "type"?
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'm not sure I understand your question. Could you please clarify.
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.
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.
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 think duplication_tag
is a better name, as it indicates what it is.
That leads to side questions:
- Should "Tag" be part of the name of the tag, as in
ScatterNonDuplicatedTag
? (It seems like extra verbosity which doesn't really convey extra meaning, so I'm leaning against it.) - Should tag types have
explicit
default constructors? When passed to functions, it disallows aggregate initialization{}
syntax. I don't think it matters at all in this case, but we should have a consistent policy towards 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.
|
Retest this please |
@jeffmiles63 build issues are related to c5e32b8#diff-90ccdebd1a5c5cce9dce2a2d6bfd4c74 that got merged in the meantime. |
d48d581
to
3d8aac7
Compare
Can you squash your changes? |
apply clang format add / fix overrides of create_scatter_view Fix issue with HPX fix comments with ScatterValue fix compile error after rebasing change types to tags Fix issue with OpenMPTarget Fix formatting for openmp target fix compilation issues with scatter view and openmptarget
8fc60fe
to
ac45b66
Compare
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Retest this please. |
…lication::{duplication_tag -> type}
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.
Still looks good to me but we could squash the last fixup commits.
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
150de6d
to
a938b2f
Compare
Update all of ScatterView and aggregate classes to use types instead of enum.
Update create_scatter_view to use types instead of enums
Add two additional create_scatter_view overloads that can derive the types:
create_scatter_view(OpType, Duplication, Contribution,View<RT, RP...> const& original_view)
create_scatter_view(OpType,View<RT, RP...> const& original_view)
Added/updated unit tests to match changes
Also cleaned up the documentation for ScatterValue classes
Part of issue #3024