kokkos
Incorrect integral type for CrsMatrix constructor?
In the constructor,
https://github.com/kokkos/kokkos-kernels/blob/e4529f966d2bad5c4cdaec6911bef44a8013d5fb/src/sparse/KokkosSparse_CrsMatrix.hpp#L618
the pointer for rowmap may need to be size_type*
@lucbv
Looking at the constructor body, it seems that we are assuming that the user will give us both row_map and colindices as OrdinalType* so this is not a bug but a conscious decision.
My guess is that if you have the data with the proper type you might be expected to wrap the pointers in views directly and use the view based constructor.
@brian-kelley @srajama1 do you recall the logic behind this decision?
A really long time ago, that interface was intended for taking COO format raw arrays, so rather than "rowmap" it was just "rows" and the documentation said COO. But the implementation always assumed CRS. In #708 I fixed the documentation but left the impl the same in case Tpetra was using it.
An easy fix might be to template that constructor on the user's rowmap element type, then it would work for both.
@brian-kelley we could do that or just add an overload, but the question really becomes: do we want to keep the current version or should we just deprecate it? Do we want the new overload or do we feel that users should do the wrapping with views themselves?
I would lean toward deprecating the current constructor as it is fairly confusing and I do not think that a new overload is really needed, we can just ask users to perform the wrapping of pointers into views directly.
I think creating a small matrix for tests is a common enough pattern to justify keeping it. If you start out with something like:
int rowmap[5] = {0, 1, 4, 6, 9};
int entries[9] = {...};
double values[9] = {...};
then this constructor does what you want in one line. Otherwise you have to wrap the data into views, and either construct a HostSpace matrix and convert, or deep_copy each view to the final memory space yourself. Either route is 5 or more extra lines.
Hum, I have mixed feelings about this, I see how this is convenient for test and examples but I suspect that if we allow this actual applications will use it and see bad performance because we are doing deep_copies under the hood. Of course they should expect it since they give us host pointers but that's never prevented people from complaining after making a mistake.
I would be more inclined to have it as a helper function in the test namespace so people are not as tempted to do this?