Implement the triangular solver #327

pratikvn · 2019-07-31T16:15:47Z

This PR implements the triangular solver in Ginkgo.

The reference and omp versions use a very simple serial version of the solver.

The CUDA version will use the level based solver.

A separate generate kernel is used to setup the matrix and explicitly check if the given matrix is a CSR matrix, if not convert to CSR. The executor specific generate kernels will setup the analysis phase if required( currently only for CUDA, but maybe in the future for OMP as well.)
Unfortunately, this means that the reference and OMP kernels will not be doing the same as the CUDA kernels, so testing the CUDA kernel with the reference will not be possible (I am not really sure how to test the CUDA generate kernel as well, but I guess that will be part of another PR).

Things to discuss:

Currently, it only accepts a lower triangular matrix. So, this may need to be updated, either with a new solver altogether for the upper triangular matrix, or using a transpose to move the lower triangular to a upper triangular (the cost is low as this would be done within the generate which should ideally be called only once), or using a parameter which sets the type of the matrix (L or U) and have two separate solve kernels for L and for U separately.
When you do a triangular solve, I think in general, you would in most cases do a L solve (forward substitution) followed by a U solve(backward substitution) for example, when you have a factorized matrix as in the case of PARILU: L (Ux) = b. First solve Ly=b for y and then solve Ux = y for x.

So, maybe we should have something that does both solves in one apply. Maybe something that takes in a Composition<LinOp> and uses it to solve for both doing consecutive trs applies. We could call it a full_apply or something like that.

core/test/solver/trs.cpp

codecov · 2019-07-31T17:28:20Z

Codecov Report

Merging #327 into develop will increase coverage by 0.15%.
The diff coverage is 98.95%.

@@             Coverage Diff             @@
##           develop     #327      +/-   ##
===========================================
+ Coverage    88.26%   88.41%   +0.15%     
===========================================
  Files          250      258       +8     
  Lines        19552    19835     +283     
===========================================
+ Hits         17257    17537     +280     
- Misses        2295     2298       +3

Impacted Files	Coverage Δ
include/ginkgo/core/base/exception_helpers.hpp	`100% <ø> (ø)`	⬆️
include/ginkgo/core/base/exception.hpp	`100% <100%> (ø)`	⬆️
core/test/utils/matrix_generator.hpp	`100% <100%> (ø)`	⬆️
reference/test/solver/lower_trs_kernels.cpp	`100% <100%> (ø)`
core/test/solver/lower_trs.cpp	`100% <100%> (ø)`
reference/test/solver/lower_trs.cpp	`100% <100%> (ø)`
core/solver/lower_trs.cpp	`100% <100%> (ø)`
include/ginkgo/core/base/polymorphic_object.hpp	`100% <100%> (ø)`	⬆️
core/test/base/exception_helpers.cpp	`100% <100%> (ø)`	⬆️
omp/solver/lower_trs_kernels.cpp	`100% <100%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd7465d...3f08cf0. Read the comment docs.

dev_tools/scripts/todo_trs.txt

tcojean

Some preliminary code cleanups I found are required.

core/solver/trs_kernels.hpp

core/test/utils/matrix_generator.hpp

cuda/solver/trs_kernels.cu

omp/test/solver/trs_kernels.cpp

reference/solver/trs_kernels.cpp

thoasm · 2019-08-05T10:26:01Z

@pratikvn We will have a preconditioner that does both solves in one apply. I am almost done with it, and I am working on it now. I is in the branch ilu_preconditioner. Currently, it can set the solvers of both L and U separately with template parameters, and I intended to use an alias for a version with the triangular solvers set.
As far as I know, a complete solver might only be useful for dense matrices since we will not have the actual (not just the partial) L and U of a sparse matrix.

pratikvn · 2019-08-05T11:51:40Z

@thoasm , a complete solver can be used for a general triangular matrix as well. Currently, I guess we do not have something to get the complete factorization of a matrix. But I think it should be possible for the user to pass in a triangular matrix (or a composition of L and U) to the trs solver and be able to get the full solution. For example, if you are dealing with SPD matrices, then you would just have a Cholesky factor (one triangular system) which you could use to solve the full matrix.

hartwiganzt · 2019-08-05T12:03:29Z

For example, if you are dealing with SPD matrices, then you would just have a Cholesky factor (one triangular system) which you could use to solve the full matrix.

We would then duplicate the triangular factor, to have separate L-solve and U-solve.

thoasm

These are the comments for half of the files, I will do the rest tomorrow.
Most of them are minor, but I would like you to change a lot related to BadDimension.

core/solver/trs.cpp

core/solver/trs_kernels.hpp

core/test/solver/trs.cpp

core/test/utils/matrix_generator.hpp

core/test/base/exception_helpers.cpp

include/ginkgo/core/base/exception_helpers.hpp

include/ginkgo/core/base/exception.hpp

core/solver/trs.cpp

thoasm

Second part of my review. Mostly style changes and some minor improvement suggestions.

include/ginkgo/core/solver/trs.hpp

core/solver/trs.cpp

omp/solver/trs_kernels.cpp

omp/test/solver/trs_kernels.cpp

reference/test/solver/trs.cpp

reference/test/solver/trs_kernels.cpp

thoasm

Mostly minor style and some documentation comments.
However, some also touch the interface (e.g. making b_ a matrix::Dense<..., ...>), which we can also discuss if you want. I feel like it is necessary.

core/solver/lower_trs.cpp

include/ginkgo/core/solver/lower_trs.hpp

core/test/solver/lower_trs.cpp

core/test/utils/matrix_generator.hpp

reference/solver/lower_trs_kernels.cpp

omp/solver/lower_trs_kernels.cpp

reference/test/solver/lower_trs.cpp

include/ginkgo/core/base/exception.hpp

include/ginkgo/core/solver/lower_trs.hpp

thoasm · 2019-08-13T17:53:48Z

When reviewing @yhmtsai's code, I saw that he also included his part into the install test.
It is the correct thing to do, but I completely forgot about that. You also need to add a small snippet into test_install/test_install.cpp to test if it was installed properly.

+ Clarifying documentation. + Setup system_matrix and rhs to be of csr and dense instead of Linop. + Modify tests acc to above issue.

Do not want to block this PR.

yhmtsai

do the tests need to be in reference/test/solver/lower_trs.cpp?
The tests, like CanBeCloned, CanBeCleared, seems to move to core test.

core/solver/lower_trs.cpp

core/test/utils/matrix_generator.hpp

include/ginkgo/core/solver/lower_trs.hpp

reference/test/solver/lower_trs.cpp

reference/test/solver/lower_trs_kernels.cpp

pratikvn · 2019-08-14T13:32:23Z

@yhmtsai , the CanBeCopied and others in reference/test/solver/trs.cpp cannot be moved to core because they need the generate method to create the solve which needs the executor. And hence this will not work when you build with no executor (As we do in some of the CI containers).

yhmtsai

LGTM.

core/test/utils/matrix_generator_test.cpp

+ Add a copy_and_convert implementation that takes shared_ptrs.

stale

thoasm

1st part of the review (split into 2 parts).

core/solver/lower_trs.cpp

include/ginkgo/core/base/polymorphic_object.hpp

include/ginkgo/core/solver/lower_trs.hpp

thoasm

These should be my last comment, it looks really good!
From my side, we are ready to merge if the remaining comments are addressed.

reference/test/solver/lower_trs.cpp

+ Fix GKO_COMMA and generate method parameter issues thanks to Thomas.

thoasm

LGTM!

hartwiganzt · 2019-08-16T13:02:12Z

Excellent! @thoasm, I count on you soon finishing the ParILU preconditioner and providing an example on how to use it.

The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.1.0. This release brings several performance improvements, adds Windows support, adds support for factorizations inside Ginkgo and a new ILU preconditioner based on ParILU algorithm, among other things. For detailed information, check the respective issue. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + Windows + MinGW and CygWin: gcc 5.3+, 6.3+, 7.3+, 8.1+ + Microsoft Visual Studio: VS 2017 15.7+ + CUDA module: CUDA 9.0+, Microsoft Visual Studio + OpenMP module: MinGW or CygWin. The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues). Additions: + Upper and lower triangular solvers ([#327](#327), [#336](#336), [#341](#341), [#342](#342)) + New factorization support in Ginkgo, and addition of the ParILU algorithm ([#305](#305), [#315](#315), [#319](#319), [#324](#324)) + New ILU preconditioner ([#348](#348), [#353](#353)) + Windows MinGW and Cygwin support ([#347](#347)) + Windows Visual studio support ([#351](#351)) + New example showing how to use ParILU as a preconditioner ([#358](#358)) + New example on using loggers for debugging ([#360](#360)) + Add two new 9pt and 27pt stencil examples ([#300](#300), [#306](#306)) + Allow benchmarking CuSPARSE spmv formats through Ginkgo's benchmarks ([#303](#303)) + New benchmark for sparse matrix format conversions ([#312](https://github.com/ginkgo-project/ginkgo/issues/312)[#317](https://github.com/ginkgo-project/ginkgo/issues/317)) + Add conversions between CSR and Hybrid formats ([#302](#302), [#310](#310)) + Support for sorting rows in the CSR format by column idices ([#322](#322)) + Addition of a CUDA COO SpMM kernel for improved performance ([#345](#345)) + Addition of a LinOp to handle perturbations of the form (identity + scalar * basis * projector) ([#334](#334)) + New sparsity matrix representation format with Reference and OpenMP kernels ([#349](#349), [#350](#350)) Fixes: + Accelerate GMRES solver for CUDA executor ([#363](#363)) + Fix BiCGSTAB solver convergence ([#359](#359)) + Fix CGS logging by reporting the residual for every sub iteration ([#328](#328)) + Fix CSR,Dense->Sellp conversion's memory access violation ([#295](#295)) + Accelerate CSR->Ell,Hybrid conversions on CUDA ([#313](#313), [#318](#318)) + Fixed slowdown of COO SpMV on OpenMP ([#340](#340)) + Fix gcc 6.4.0 internal compiler error ([#316](#316)) + Fix compilation issue on Apple clang++ 10 ([#322](#322)) + Make Ginkgo able to compile on Intel 2017 and above ([#337](#337)) + Make the benchmarks spmv/solver use the same matrix formats ([#366](#366)) + Fix self-written isfinite function ([#348](#348)) + Fix Jacobi issues shown by cuda-memcheck Tools and ecosystem: + Multiple improvements to the CI system and tools ([#296](#296), [#311](#311), [#365](#365)) + Multiple improvements to the Ginkgo containers ([#328](#328), [#361](#361)) + Add sonarqube analysis to Ginkgo ([#304](#304), [#308](#308), [#309](#309)) + Add clang-tidy and iwyu support to Ginkgo ([#298](#298)) + Improve Ginkgo's support of xSDK M12 policy by adding the `TPL_` arguments to CMake ([#300](#300)) + Add support for the xSDK R7 policy ([#325](#325)) + Fix examples in html documentation ([#367](#367))

The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.1.0. This release brings several performance improvements, adds Windows support, adds support for factorizations inside Ginkgo and a new ILU preconditioner based on ParILU algorithm, among other things. For detailed information, check the respective issue. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + Windows + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, 8.1+ + Microsoft Visual Studio: VS 2017 15.7+ + CUDA module: CUDA 9.0+, Microsoft Visual Studio + OpenMP module: MinGW or Cygwin. The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues). ### Additions + Upper and lower triangular solvers ([#327](#327), [#336](#336), [#341](#341), [#342](#342)) + New factorization support in Ginkgo, and addition of the ParILU algorithm ([#305](#305), [#315](#315), [#319](#319), [#324](#324)) + New ILU preconditioner ([#348](#348), [#353](#353)) + Windows MinGW and Cygwin support ([#347](#347)) + Windows Visual Studio support ([#351](#351)) + New example showing how to use ParILU as a preconditioner ([#358](#358)) + New example on using loggers for debugging ([#360](#360)) + Add two new 9pt and 27pt stencil examples ([#300](#300), [#306](#306)) + Allow benchmarking CuSPARSE spmv formats through Ginkgo's benchmarks ([#303](#303)) + New benchmark for sparse matrix format conversions ([#312](https://github.com/ginkgo-project/ginkgo/issues/312)[#317](https://github.com/ginkgo-project/ginkgo/issues/317)) + Add conversions between CSR and Hybrid formats ([#302](#302), [#310](#310)) + Support for sorting rows in the CSR format by column idices ([#322](#322)) + Addition of a CUDA COO SpMM kernel for improved performance ([#345](#345)) + Addition of a LinOp to handle perturbations of the form (identity + scalar * basis * projector) ([#334](#334)) + New sparsity matrix representation format with Reference and OpenMP kernels ([#349](#349), [#350](#350)) ### Fixes + Accelerate GMRES solver for CUDA executor ([#363](#363)) + Fix BiCGSTAB solver convergence ([#359](#359)) + Fix CGS logging by reporting the residual for every sub iteration ([#328](#328)) + Fix CSR,Dense->Sellp conversion's memory access violation ([#295](#295)) + Accelerate CSR->Ell,Hybrid conversions on CUDA ([#313](#313), [#318](#318)) + Fixed slowdown of COO SpMV on OpenMP ([#340](#340)) + Fix gcc 6.4.0 internal compiler error ([#316](#316)) + Fix compilation issue on Apple clang++ 10 ([#322](#322)) + Make Ginkgo able to compile on Intel 2017 and above ([#337](#337)) + Make the benchmarks spmv/solver use the same matrix formats ([#366](#366)) + Fix self-written isfinite function ([#348](#348)) + Fix Jacobi issues shown by cuda-memcheck ### Tools and ecosystem improvements + Multiple improvements to the CI system and tools ([#296](#296), [#311](#311), [#365](#365)) + Multiple improvements to the Ginkgo containers ([#328](#328), [#361](#361)) + Add sonarqube analysis to Ginkgo ([#304](#304), [#308](#308), [#309](#309)) + Add clang-tidy and iwyu support to Ginkgo ([#298](#298)) + Improve Ginkgo's support of xSDK M12 policy by adding the `TPL_` arguments to CMake ([#300](#300)) + Add support for the xSDK R7 policy ([#325](#325)) + Fix examples in html documentation ([#367](#367)) Related PR: #370

pratikvn requested review from thoasm, hartwiganzt and tcojean July 31, 2019 16:16

pratikvn self-assigned this Jul 31, 2019

pratikvn force-pushed the trs_solver branch from 83ceca6 to 205c459 Compare July 31, 2019 16:36

tcojean reviewed Jul 31, 2019

View reviewed changes

core/test/solver/trs.cpp Outdated Show resolved Hide resolved

tcojean reviewed Jul 31, 2019

View reviewed changes

core/test/solver/trs.cpp Outdated Show resolved Hide resolved

tcojean reviewed Aug 1, 2019

View reviewed changes

dev_tools/scripts/todo_trs.txt Outdated Show resolved Hide resolved

pratikvn marked this pull request as ready for review August 1, 2019 13:45

pratikvn requested a review from tcojean August 1, 2019 13:45

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 Aug 1, 2019

pratikvn force-pushed the trs_solver branch from bdf36f4 to 1bb5283 Compare August 1, 2019 16:29

tcojean previously requested changes Aug 1, 2019

View reviewed changes

thoasm requested changes Aug 5, 2019

View reviewed changes

thoasm requested changes Aug 6, 2019

View reviewed changes

pratikvn force-pushed the trs_solver branch from 9579cd5 to b6566e6 Compare August 8, 2019 16:17

pratikvn added the 1:ST:do-not-merge Please do not merge PR this yet. label Aug 8, 2019

pratikvn force-pushed the trs_solver branch from b6566e6 to b13be58 Compare August 9, 2019 08:03

pratikvn requested a review from thoasm August 9, 2019 09:09

pratikvn force-pushed the trs_solver branch from 265ccbb to 217a9c1 Compare August 12, 2019 12:30

thoasm previously requested changes Aug 12, 2019

View reviewed changes

thoasm requested a review from yhmtsai August 13, 2019 09:23

yhmtsai requested changes Aug 13, 2019

View reviewed changes

include/ginkgo/core/base/exception.hpp Show resolved Hide resolved

include/ginkgo/core/solver/lower_trs.hpp Outdated Show resolved Hide resolved

pratikvn added 2 commits August 14, 2019 10:48

Fix some review comments.

cac1b68

+ Clarifying documentation. + Setup system_matrix and rhs to be of csr and dense instead of Linop. + Modify tests acc to above issue.

Newline, doc align and test-install.

8fbad33

pratikvn force-pushed the trs_solver branch from e1b8e0e to 8fbad33 Compare August 14, 2019 08:59

yhmtsai requested changes Aug 14, 2019

View reviewed changes

Add lower tr mat gen with ones on diag. Some review comments.

a4d978e

pratikvn removed the 1:ST:do-not-merge Please do not merge PR this yet. label Aug 14, 2019

yhmtsai approved these changes Aug 15, 2019

View reviewed changes

core/test/utils/matrix_generator_test.cpp Show resolved Hide resolved

Fix the copy_and_convert_to bug.

58e59f9

+ Add a copy_and_convert implementation that takes shared_ptrs.

pratikvn requested a review from thoasm August 15, 2019 14:50

thoasm reviewed Aug 15, 2019

View reviewed changes

reference/test/solver/lower_trs.cpp Outdated Show resolved Hide resolved

thoasm reviewed Aug 15, 2019

View reviewed changes

reference/test/solver/lower_trs.cpp Outdated Show resolved Hide resolved

Fix some final review comments.

3f08cf0

+ Fix GKO_COMMA and generate method parameter issues thanks to Thomas.

pratikvn force-pushed the trs_solver branch from 5250aa4 to 3f08cf0 Compare August 15, 2019 17:48

thoasm approved these changes Aug 16, 2019

View reviewed changes

pratikvn merged commit ef5c9c2 into develop Aug 16, 2019

pratikvn deleted the trs_solver branch August 16, 2019 09:52

tcojean 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 Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the triangular solver #327

Implement the triangular solver #327

pratikvn commented Jul 31, 2019

codecov bot commented Jul 31, 2019 •

edited

Loading

tcojean left a comment

thoasm commented Aug 5, 2019

pratikvn commented Aug 5, 2019

hartwiganzt commented Aug 5, 2019 •

edited

Loading

thoasm left a comment

thoasm left a comment

thoasm left a comment

thoasm commented Aug 13, 2019

yhmtsai left a comment

pratikvn commented Aug 14, 2019

yhmtsai left a comment

thoasm left a comment

thoasm left a comment

thoasm left a comment

hartwiganzt commented Aug 16, 2019

Implement the triangular solver #327

Implement the triangular solver #327

Conversation

pratikvn commented Jul 31, 2019

codecov bot commented Jul 31, 2019 • edited Loading

Codecov Report

tcojean left a comment

Choose a reason for hiding this comment

thoasm commented Aug 5, 2019

pratikvn commented Aug 5, 2019

hartwiganzt commented Aug 5, 2019 • edited Loading

thoasm left a comment

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

thoasm commented Aug 13, 2019

yhmtsai left a comment

Choose a reason for hiding this comment

pratikvn commented Aug 14, 2019

yhmtsai left a comment

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

thoasm left a comment

Choose a reason for hiding this comment

hartwiganzt commented Aug 16, 2019

codecov bot commented Jul 31, 2019 •

edited

Loading

hartwiganzt commented Aug 5, 2019 •

edited

Loading