Pmis by Arianna Amadini and Daniele Salvi #1922
Conversation
There was a problem hiding this comment.
Thanks for contribution.
We have auto formatting tool pre-commit. It will help your code to fit our guideline.
after you install pre-commit, you can run pre-commit install to install our formatter into ginkgo source folder.
It will format your code when you commit something.
to format the files already in commits, you can run pre-commit run --from-ref <reference> --to-ref HEAD i.e. pre-commit run --from-ref origin/pmis--to-ref HEAD
| #include "core/base/array_access.hpp" | ||
| #include "core/components/prefix_sum_kernels.hpp" | ||
|
|
||
| #include <random> |
There was a problem hiding this comment.
Could you revert the changes in this files?
Unified kernels need to use the macro to adapt different backend.
cpu-only code won't work here properly
| @@ -1,3 +1,2 @@ | |||
| ginkgo_create_test(pgm_kernels) | |||
| ginkgo_create_test(pmis_kernels) | |||
| A->read({{3, 3}, | ||
| {{0, 0, value_type{2}}, | ||
| {0, 1, value_type{-1}}, | ||
| {0, 2, value_type{0}}, |
There was a problem hiding this comment.
is zero a specific design for the testing?
|
|
||
| TYPED_TEST(Pmis, ComputeStrongDepRow1) | ||
| { | ||
| using value_type = typename TestFixture::value_type; |
There was a problem hiding this comment.
Except for some tests, we usually follow Arrange-Act-Assert pattern in test, which having one empty break between Arrange/Act/Assert.
you can use comment in the block if you want to separate some details.
| std::array<index_type, 4> expected{index_type{0}, index_type{2}, | ||
| index_type{3}, index_type{4}}; |
There was a problem hiding this comment.
| std::array<index_type, 4> expected{index_type{0}, index_type{2}, | |
| index_type{3}, index_type{4}}; | |
| gko::array<index_type> expected(this->exec, {0, 2, 3, 4}; |
and then later you can use
GKO_ASSERT_ARRAY_EQ(sparsity_rows, expected);
same for the other test.
note: expected can be considered in "Arrange" section
|
|
||
| for (auto j = row_start; j < row_end; ++j) { | ||
| const auto k = col_idxs[j]; | ||
| if (k == i) continue; |
There was a problem hiding this comment.
| if (k == i) continue; | |
| if (k == i) {continue;} |
It is from our format guildline.
we always use {} even when it only contains one line.
| std::vector<IndexType> row_offsets(csr->get_size()[0]); | ||
| for (IndexType row = 0; row < csr->get_size()[0]; ++row) { | ||
| row_offsets[row] = strong_dep->get_row_ptrs()[row]; | ||
| } |
There was a problem hiding this comment.
it is unnecessary.
In your code, you only need to get it on betweb line 107 and 108 as temporary variable and work on it.
| const auto nrows = static_cast<IndexType>(strong_dep->get_size()[0]); | ||
| const auto s_row_ptrs = strong_dep->get_const_row_ptrs(); | ||
| const auto s_col_idxs = strong_dep->get_const_col_idxs(); | ||
| std::vector<char> higher(nrows, 0); |
|
|
||
| for (IndexType i=0; i < nrows; i++){ | ||
| if (status[i] == 0) { | ||
| char check = 'c'; |
There was a problem hiding this comment.
check gives one of 'a', 'b', 'c' without the meaning, which is hard to maintain in the future.
At least, you should give the meaning in the comment.
I think the following code can just rely on status without check
| for (auto r = 0; r < nrows; ++r) | ||
| weight[r] = rc{0}; |
There was a problem hiding this comment.
| for (auto r = 0; r < nrows; ++r) | |
| weight[r] = rc{0}; | |
| for (auto r = 0; r < nrows; ++r) | |
| { | |
| weight[r] = rc{0}; | |
| } |
|
Because the pr author are busy, this pr is wip and merged to another working branch, I will merge it and take care of the changes. |
2 participants
This PR introduces the first part of the PMIS algorithm, focusing on the identification of coarse and fine nodes in a sparse matrix.