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

Kokkos static CRS graph row view is too tightly typed #5939

Open
maartenarnst opened this issue Mar 2, 2023 · 2 comments
Open

Kokkos static CRS graph row view is too tightly typed #5939

maartenarnst opened this issue Mar 2, 2023 · 2 comments
Labels
Enhancement Improve existing capability; will potentially require voting Question For Kokkos internal and external contributors and users

Comments

@maartenarnst
Copy link
Contributor

maartenarnst commented Mar 2, 2023

During siam cse, we discussed with @brian-kelley about using static crs graph with a custom type as the data type.

issue 1

This led us to look at such a use, which led us to the following line in the static crs graph row view helper structure:

using ordinal_type = const typename GraphType::data_type;

It is used afterwards in the ‘colidx’ getter both as the ordinal type for the index and as the value type for the return value. This seems appropriate if the data type is an integral type. But, in a use case in which it’s for instance a floating type or a custom type, it seems too restrictive. It seems it prevents static crs graph to be used with non integral data type.

Similarly, in the static crs graph class itself, there seem to be similar issues, where the data type is also used as the ordinal type.

Issue 2

We also wonder why the row view structure is used. Since the stride is always 1, it seems that a kokkos subview could be used instead too.

return GraphRowViewConst<StaticCrsGraph>(entries, 1, count, start);

@brian-kelley
Copy link
Contributor

Thanks for writing this @maartenarnst .

I guess the main questions for Kokkos core are:

  • is it appropriate to let StaticCrsGraph be a more general 2D ragged array type, instead of being only for storing a graph as a sparse adjacency matrix, and
  • if not, should there be a separate type for 2D ragged arrays, using offsets+data?

It seems to me that relaxing the assumption that data_type is an integer (option 1) doesn't seem very difficult.

@ajpowelsnl ajpowelsnl added Question For Kokkos internal and external contributors and users Enhancement Improve existing capability; will potentially require voting labels Mar 6, 2023
@crtrott
Copy link
Member

crtrott commented Mar 21, 2023

I think we can consider this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting Question For Kokkos internal and external contributors and users
Projects
None yet
Development

No branches or pull requests

4 participants