-
Notifications
You must be signed in to change notification settings - Fork 98
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
Adopt Kokkos::StaticCrsGraph
from "Core" and move Static{Ccs,Crs}Graph
to namespace KokkosSparse::
#2419
base: develop
Are you sure you want to change the base?
Conversation
* Moved Static{Ccs,Crs}Graph to namespace KokkosSparse and into their own new header files * Deprecate symbols in Kokkos namespace * CRS counterpart used to come from the Containers subpackage in Kokkos "Core" but now is transitioned to Kokkos Kernels. Signed-off-by: Damien L-G <[email protected]>
Signed-off-by: Damien L-G <[email protected]>
using Kokkos::create_staticcrsgraph; | ||
using Kokkos::GraphRowViewConst; | ||
using Kokkos::maximum_entry; | ||
using Kokkos::StaticCrsGraph; |
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 am not sure what to do about create_mirror[_view]
I certainly want to pull all overloads from Kokkos into KokkosSparse::
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 don't think Kokkos provides create_mirror...
functions for any other non-view containers, right? Shall we just be consistent with that and move them into KokkosSparse
namespace? Kokkos can make the suggestion in the deprecation message.
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.
Approving to get some CI going
|
||
} /* namespace TestStaticCrsGraph */ | ||
|
||
TEST(NotSureWhatGoesThere, staticcrsgraph) { |
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.
Need to fix this before we merge
Let me know how you would like to call 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.
We just put TestCategory
and it gets set in e.g. test_common/Test_HIP.hpp for different backends.
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.
Looks like this still needs to be addressed, adding Carl's advice as a code suggestion:
TEST(NotSureWhatGoesThere, staticcrsgraph) { | |
TEST(TestCategory, staticcrsgraph) { |
…to static_crs_graph
kokkos-kernels nightly builds with deprecated code off are failing after merge of kokkos/kokkos#7516 , I'm assuming merge of this PR is needed to get develop branches back in sync? For reference, this is the error:
|
The issues with runners should be resolved (killed some of the previous runs) |
This PR implements what we agreed on, that is Kokkos "Core" is deprecating
Kokkos::StaticCrsGraph
and we transition it to Kokkos Kernels intoKokkosSparse::
.On 2nd thought we decided to give up on raising deprecation warnings because it was too tricky to guarantee same symbols and not emit diagnostics when the KokkosSparse:: versions are used.