Add a batched block Jacobi preconditioner #1542

pratikvn · 2024-02-14T09:53:47Z

This PR adds a batched block Jacobi preconditioner. This PR only adds reference and omp kernels to reduce the amount of code that needs to be reviewed. Again, as usual, a lot of it is harness code,

TODO

Update docs
Format tests

yhmtsai

first part of my reviews

include/ginkgo/core/preconditioner/batch_jacobi.hpp

yhmtsai · 2024-02-19T10:27:02Z

include/ginkgo/core/preconditioner/batch_jacobi.hpp

+         * where the first value is 0, and the last value is the number of
+         * rows / columns of the matrix.
+         *
+         * @note Even if not set explicitly, this parameter will be set to


do we make the parameters in non-batch method non-mutable usage although it is still marked as mutable variable

I dont think it should be marked mutable ?

include/ginkgo/core/preconditioner/batch_jacobi.hpp

reference/test/preconditioner/batch_jacobi_kernels.cpp

reference/preconditioner/batch_jacobi_kernels.hpp.inc

MarcelKoch

My first comments just on the interface.

include/ginkgo/core/preconditioner/batch_jacobi.hpp

MarcelKoch

Second part. Mostly minor stuff.

The largest issue for me is still the storage_scheme type. I would like a better solution to it, either storing the data it needs in the class, or freestanding functions.

MarcelKoch · 2024-02-19T14:54:21Z

core/matrix/batch_struct.hpp

@@ -119,6 +130,11 @@ struct batch_item {
    index_type num_rows;
    index_type num_cols;
    index_type num_stored_elems_per_row;
+
+    inline size_type get_single_item_num_nnz() const


I find it a bit odd, that both uniform_batch and batch_item have both the function get_single_item_num_nnz, but TBH I'm not sure what to do about that. Maybe only keep the batch_item one and use extract_batch_item?

core/test/preconditioner/batch_jacobi.cpp

reference/preconditioner/batch_block_jacobi.hpp

reference/preconditioner/batch_scalar_jacobi.hpp

test/preconditioner/batch_jacobi_kernels.cpp

omp/preconditioner/batch_jacobi_kernels.cpp

reference/preconditioner/batch_jacobi_kernels.hpp.inc

omp/preconditioner/batch_jacobi_kernels.cpp

reference/preconditioner/batch_scalar_jacobi.hpp

.pre-commit-config.yaml

core/preconditioner/batch_jacobi.cpp

stale

MarcelKoch

mostly smaller nits left. One larger thing is where should the block pointers should be stored. I would suggest the Jacobi class directly, instead of the parameters.

include/ginkgo/core/preconditioner/batch_jacobi.hpp

reference/preconditioner/batch_jacobi_kernels.hpp.inc

core/preconditioner/batch_jacobi_kernels.hpp

test/preconditioner/batch_jacobi_kernels.cpp

MarcelKoch · 2024-04-17T09:48:48Z

reference/test/preconditioner/batch_jacobi_kernels.cpp

+
+    for (int k = 0; k < num_blocks; k++) {
+        const auto bsize = block_ptrs[k + 1] - block_ptrs[k];
+        for (int r = 0; r < bsize; r++) {


also here, maybe use dense matrix views.

Is there a reason it's not possible to use the views here?

We compare the unbatched version with the batched one, the storage of the blocks is different. Additionally, for debugging purposes, it is easier if there is a element based comparison in case there are issues.

test/solver/batch_bicgstab_kernels.cpp

core/preconditioner/batch_jacobi_helpers.hpp

Co-authored-by: Isha Aggarwal<isha.aggarwal2@kit.edu> Co-authored-by: Aditya Kashi<kashia@ornl.gov>

Co-authored-by: Marcel Koch <marcel.koch@kit.edu> Co-authored-by: Yu-Hsiang Tsai <yhmtsai@gmail.com>

due to circ dep issues, libginkgo -> libginkgo_kernels

Co-authored-by: Yu-Hsiang Tsai <yhmtsai@gmail.com>

pratikvn added 1:ST:WIP This PR is a work in progress. Not ready for review. type:batched-functionality This is related to the batched functionality in Ginkgo labels Feb 14, 2024

pratikvn self-assigned this Feb 14, 2024

ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. type:preconditioner This is related to the preconditioners mod:all This touches all Ginkgo modules. labels Feb 14, 2024

pratikvn force-pushed the batch-prec-jacobi branch 2 times, most recently from 7de7adf to 6d06c91 Compare February 14, 2024 11:37

pratikvn changed the title ~~WIP: Add a batched block Jacobi preconditioner~~ Add a batched block Jacobi preconditioner Feb 14, 2024

pratikvn requested review from a team February 14, 2024 18:30

pratikvn added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Feb 14, 2024

yhmtsai reviewed Feb 19, 2024

View reviewed changes

MarcelKoch reviewed Feb 19, 2024

View reviewed changes

MarcelKoch previously requested changes Feb 19, 2024

View reviewed changes

pratikvn force-pushed the batch-prec-jacobi branch from 612b423 to a1431e2 Compare February 22, 2024 09:18

yhmtsai reviewed Mar 26, 2024

View reviewed changes

pratikvn force-pushed the batch-prec-jacobi branch 2 times, most recently from 233c13c to fe0507d Compare April 2, 2024 14:40

MarcelKoch added this to the Ginkgo 1.8.0 milestone Apr 5, 2024

pratikvn force-pushed the batch-prec-jacobi branch 2 times, most recently from d8b398d to 43c4797 Compare April 17, 2024 07:17

pratikvn requested review from yhmtsai and MarcelKoch April 17, 2024 07:18

MarcelKoch previously requested changes Apr 17, 2024

View reviewed changes

pratikvn force-pushed the batch-prec-jacobi branch from 43c4797 to 9da3d7e Compare April 18, 2024 10:15

pratikvn requested a review from MarcelKoch April 18, 2024 12:14

pratikvn force-pushed the batch-prec-jacobi branch from 32de29b to fcff54f Compare May 6, 2024 22:59

yhmtsai approved these changes May 8, 2024

View reviewed changes

core/preconditioner/batch_jacobi_helpers.hpp Outdated Show resolved Hide resolved

core/preconditioner/batch_jacobi_helpers.hpp Outdated Show resolved Hide resolved

pratikvn added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels May 8, 2024

pratikvn and others added 23 commits May 9, 2024 15:26

Add batch::Jacobi class and core

2defd18

Co-authored-by: Isha Aggarwal<isha.aggarwal2@kit.edu> Co-authored-by: Aditya Kashi<kashia@ornl.gov>

Add core tests

2bc540e

Co-authored-by: Isha Aggarwal<isha.aggarwal2@kit.edu> Co-authored-by: Aditya Kashi<kashia@ornl.gov>

Add reference dev classes and kernels

c67e2b8

Co-authored-by: Isha Aggarwal<isha.aggarwal2@kit.edu> Co-authored-by: Aditya Kashi<kashia@ornl.gov>

Add reference tests

212b544

Co-authored-by: Isha Aggarwal<isha.aggarwal2@kit.edu> Co-authored-by: Aditya Kashi<kashia@ornl.gov>

Add omp kernels and unified tests

ebd3c5d

Co-authored-by: Isha Aggarwal<isha.aggarwal2@kit.edu> Co-authored-by: Aditya Kashi<kashia@ornl.gov>

Add device stubs

18cad0f

Co-authored-by: Isha Aggarwal<isha.aggarwal2@kit.edu> Co-authored-by: Aditya Kashi<kashia@ornl.gov>

fix typos and add batch_item nnz getter

b9eeab8

update batch_item struct

e16098b

Format tests and fix docs

4b450f5

review updates

2bf139c

Co-authored-by: Marcel Koch <marcel.koch@kit.edu> Co-authored-by: Yu-Hsiang Tsai <yhmtsai@gmail.com>

Move storage_scheme funcs to internal headers

4263556

Add jacobi precond to solver dispatch and test

14d6e70

Rem batch_jacobi_apply, add solver-precond test

f0b471d

add dev classes, update dispatch

7903d8d

wip review updates

46622e5

add assert_near for arrays

26dc67c

review updates

31d8997

s/cumulative_storage/cumulative_offsets

7642294

move block_pointers to class

f50d306

review updates

a7c8b71

Co-authored-by: Marcel Koch <marcel.koch@kit.edu> Co-authored-by: Yu-Hsiang Tsai <yhmtsai@gmail.com>

getters need to be in headers

0b1aa1c

due to circ dep issues, libginkgo -> libginkgo_kernels

add scalar_jac solver test

5938aa7

doc fixes

83a9280

Co-authored-by: Yu-Hsiang Tsai <yhmtsai@gmail.com>

pratikvn force-pushed the batch-prec-jacobi branch from 5504e62 to 83a9280 Compare May 9, 2024 13:27

pratikvn merged commit 1dfa912 into develop May 9, 2024
12 of 15 checks passed

pratikvn deleted the batch-prec-jacobi branch May 9, 2024 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a batched block Jacobi preconditioner #1542

Add a batched block Jacobi preconditioner #1542

pratikvn commented Feb 14, 2024 •

edited

Loading

yhmtsai left a comment

yhmtsai Feb 19, 2024

pratikvn Apr 2, 2024

MarcelKoch left a comment

MarcelKoch left a comment

MarcelKoch Feb 19, 2024

MarcelKoch left a comment

MarcelKoch Apr 17, 2024

MarcelKoch Apr 18, 2024

pratikvn Apr 18, 2024

Add a batched block Jacobi preconditioner #1542

Add a batched block Jacobi preconditioner #1542

Conversation

pratikvn commented Feb 14, 2024 • edited Loading

TODO

yhmtsai left a comment

Choose a reason for hiding this comment

yhmtsai Feb 19, 2024

Choose a reason for hiding this comment

pratikvn Apr 2, 2024

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

MarcelKoch Feb 19, 2024

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

MarcelKoch Apr 17, 2024

Choose a reason for hiding this comment

MarcelKoch Apr 18, 2024

Choose a reason for hiding this comment

pratikvn Apr 18, 2024

Choose a reason for hiding this comment

pratikvn commented Feb 14, 2024 •

edited

Loading