MT Gauss-Seidel fixups #255
Conversation
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Matrix market writers live in test_common/ . I assume this is for debugging , but still having two write functions seems redundant.
There was a problem hiding this comment.
Oh, I didn't see those! Then there is obviously no reason to add this again.
| // Write the dimensions of the sparse matrix: (# rows, # | ||
| // columns, # matrix entries (counting duplicates as | ||
| // separate entries)). | ||
| out << numRows << " " << 1 << " " << endl; |
There was a problem hiding this comment.
Sorry to nit pick, space in the end of line ?
| // Write the dimensions of the sparse matrix: (# rows, # | ||
| // columns, # matrix entries (counting duplicates as | ||
| // separate entries)). | ||
| out << numRows << " " << 1 << " " << endl; |
There was a problem hiding this comment.
Sorry to nit pick, space in the end of line ?
| @@ -247,12 +247,12 @@ class GaussSeidelHandle{ | |||
| void set_old_to_new_map(const nnz_lno_persistent_work_view_t &old_to_new_map_) { | |||
| this->old_to_new_map = old_to_new_map_; | |||
| } | |||
| void set_permuted_diagonals (const scalar_persistent_work_view_t permuted_diagonals_){ | |||
| this->permuted_diagonals = permuted_diagonals_; | |||
| void set_permuted_inverse_diagonal (const scalar_persistent_work_view_t permuted_inverse_diagonal_){ | |||
There was a problem hiding this comment.
Does Ifpack2 already call set_permuted_diagonals ?
There was a problem hiding this comment.
No, it doesn't. This method only sets the permuted diagonal, it does not do any permutations.
| static_assert (std::is_same<typename KernelHandle::const_nnz_scalar_t, | ||
| typename scalar_nnz_view_t_::const_value_type>::value, | ||
| "KokkosSparse::gauss_seidel_symbolic: scalar type of the matrix should be same as kernelHandle scalar_t."); | ||
|
|
There was a problem hiding this comment.
This is mainly pattern match, so I assume this is ok.
|
@srajama1 I removed the matrix market routines and took care of all the whitespace and indentation issues. This blows up the diff quite significantly, but there are no functional changes. I ran the unit test on white and bowman. Anything else I should be running? |
| using namespace KokkosSparse::Impl; | ||
|
|
||
| GAUSS_SEIDEL_NUMERIC<const_handle_type, Internal_alno_row_view_t_, Internal_alno_nnz_view_t_, Internal_ascalar_nnz_view_t_>::gauss_seidel_numeric | ||
| (&tmp_handle, num_rows, num_cols, const_a_r, const_a_l, const_a_v, const_a_d, is_graph_symmetric); |
There was a problem hiding this comment.
@srajama1 The diagonal is passed here as const_a_d.
There was a problem hiding this comment.
Thanks, missed the basic thing.
It works better when you don't hard code the damping factor..
|
Will review this after a meeting. |
|
A unit test in KK will be nice as it gets tested on lot more compilers and platforms than Ifpack2. |
|
@srajama1 I changed the unit test so it would run with damping. I'll build and test on Bowman and White.. |
|
@srajama1 Tests pass on Bowman and White. |
There was a problem hiding this comment.
Sorry for my tardiness in reviewing this. I asked for a change of spacing issues and made it complicated for myself :(. Two questions:
- Are we absolutely sure this works ? I am worried about the code that saves the diagonal position and updates that position later. That position is no longer the diagonal right ? Should we pass the position of the diagonal too ?
- Is there a difference between symmetric and unsymmetric ?
|
The more I think about it, the less I am convinced that this is correct. It will not bug out, as it did before, but it probably is still not quite correct. The graph that is used for the coloring is given in terms of local row and column IDs as well, so the coloring might be off. I'll look into it when I have time. |
|
The coloring is on symmetric graph and only on the local part. Assuming there is no unsymmetric ordering of local rows and local columns it should be fine. If row i and column i are not the same vertex then we are coloring what is just in the row adjacency and ignoring any mismatch in column adjacency. |
|
@srajama1 Do you think this can be merged? |
|
Let us fix column map != row map outside KK. We can merge this. |
The last item is necessary for MT GS to work correctly with Ifpack2. The diagonal of a Tpetra matrix might not be given by pairs (i, i) of local IDs (depending on the rowmap and the colmap). Moreover, this allows to use MT GS with the l1 option from Ifpack2.
I also added some routines to dump to MatrixMarket format.
This addresses Issues #221 and #240