Sparsity format Reference and OpenMP kernels #350

pratikvn · 2019-09-12T22:36:37Z

This PR adds the Reference and OpenMP kernels for the Sparsity matrix format including their tests.

Also adds conversion from CSR and Dense to Sparsity pattern (which makes testing easier)
Fixes some minor typos in CSR-Dense conversion tests.

TODO

Merge Sparsity pattern matrix format #349 first.

codecov · 2019-09-12T23:53:26Z

Codecov Report

Merging #350 into develop will decrease coverage by 8.6%.
The diff coverage is 99.13%.

@@             Coverage Diff             @@
##           develop     #350      +/-   ##
===========================================
- Coverage    97.99%   89.38%   -8.61%     
===========================================
  Files          256      285      +29     
  Lines        19216    21845    +2629     
===========================================
+ Hits         18830    19526     +696     
- Misses         386     2319    +1933

Impacted Files	Coverage Δ
include/ginkgo/core/matrix/dense.hpp	`100% <ø> (ø)`	⬆️
include/ginkgo/core/matrix/csr.hpp	`85.48% <ø> (ø)`	⬆️
reference/test/matrix/dense_kernels.cpp	`100% <100%> (ø)`	⬆️
include/ginkgo/core/matrix/sparsity_csr.hpp	`100% <100%> (ø)`
omp/test/matrix/sparsity_csr_kernels.cpp	`100% <100%> (ø)`
reference/test/matrix/sparsity_csr_kernels.cpp	`100% <100%> (ø)`
core/test/matrix/sparsity_csr.cpp	`100% <100%> (ø)`
reference/test/matrix/sparsity_csr.cpp	`100% <100%> (ø)`
reference/matrix/sparsity_csr_kernels.cpp	`100% <100%> (ø)`
cuda/test/matrix/csr_kernels.cpp	`100% <100%> (ø)`	⬆️
... and 45 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 4776504...745eb45. Read the comment docs.

+ Fix bugs in diag removal, add more tests. + Fix value setting in CSR and dense conversions to Sparsity

yhmtsai · 2019-10-01T16:04:57Z

I think we still need Spmv when Sparsity is used as part of the stencil matrix.

pratikvn · 2019-10-02T08:18:37Z

I am not sure if this can be used as a Stencil matrix. A stencil matrix, I think would need atleast 2 values (which would be different) and hence would not be representable within sparsity. Additionally, I think we would be violating the single responsibility principle, if we try to represent the stencil matrix within sparsity. It would be much better to have a separate stencil matrix class.

thoasm · 2019-10-02T08:51:48Z

@pratikvn I agree that the Stencil matrix should be its own class.
I don't know if we might need it in ParILUT, but I would definitively not exclude the possibility that we might need it in the future, which is why I would also prefer SparsityCsr as a name.

yhmtsai · 2019-10-02T13:19:19Z

I am unclear about why we set the value (1, -1, or 1/nnz). Is It only for converting another format?

pratikvn · 2019-10-02T13:25:55Z

I guess it is to have some generality when creating the Sparsity matrix. But in almost all cases we should be using the default value of 1.0, because that is what the Sparsity matrix is for. But I guess, it will not hurt to have an SpMV implementation, unless it does confuse the user about what exactly Sparsity should be used for.

yhmtsai · 2019-10-02T13:43:47Z

If Sparsity doesn't need SPMV, is it possible to handle the sparsity inside a class like Transposable?
Maybe needs delete zeros elements, to_adjacency_matrix (delete diagonal elements), get_sparsity_value.
Every matrix format inherits the sparsity class and implement their own function.
It can reduce a lot of duplicated code but it can not allows to use Sparsity only.

pratikvn · 2019-10-02T16:14:11Z

Yes, I did think about that, adding an interface similar to Transposable. To me that still kind of violates the single responsibility principle as now you are making the the sparsity matrix piggyback on the CSR(or any other) format. I think it is easier and clear if the Sparsity is a different format as it needs to be stored as a separate matrix anyway.

thoasm

One recommended name change and some comments about the apply functionality.
Overall, it looks good, but I would like to have the comments addressed before I approve it.

core/matrix/dense.cpp

core/matrix/dense_kernels.hpp

cuda/matrix/dense_kernels.cu

omp/matrix/sparsity_csr_kernels.cpp

reference/matrix/sparsity_csr_kernels.cpp

reference/test/matrix/sparsity_csr.cpp

reference/test/matrix/sparsity_csr_kernels.cpp

yhmtsai

LGTM. Only one thing I would like to change

core/matrix/csr.cpp

tcojean

Missing conversion tests, otherwise LGTM.

core/matrix/dense.cpp

thoasm

LGTM!
However, according to Code coverage, you are still missing convert_to and move_to from Dense to SparsityCsr for OpenMP.

+ Update some forgotten names. + Improve some kernels. + Add some dense to sparsity csr conversion tests.

thoasm

LGTM!

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](#312)) + 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](#312)) + 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 self-assigned this Sep 12, 2019

pratikvn requested review from thoasm, yhmtsai, tcojean and hartwiganzt September 13, 2019 07:11

pratikvn added 1:ST:ready-for-review This PR is ready for review 1:ST:do-not-merge Please do not merge PR this yet. and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Sep 13, 2019

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from a8d9b40 to 29538ac Compare September 13, 2019 14:14

pratikvn mentioned this pull request Sep 17, 2019

Add a new permutation matrix format #352

Merged

2 tasks

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from 29538ac to 2184e57 Compare September 18, 2019 21:35

pratikvn mentioned this pull request Sep 19, 2019

Sparsity pattern matrix format #349

Merged

1 task

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch 9 times, most recently from e126104 to 712e019 Compare September 26, 2019 19:35

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch 2 times, most recently from 9c6126e to 8264dfd Compare September 30, 2019 08:28

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

pratikvn added 3 commits September 30, 2019 22:55

Add conversions to Sparsity matrix format from CSR for all execs.

03d28d3

Add a new constructor that automatically converts from the input LinOp.

31fcefd

Add ref and omp count diag kernel, tests for rem diag kernel.

01e7989

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from 27078b3 to 8873ee5 Compare September 30, 2019 20:56

Review update

8a588b8

+ Fix bugs in diag removal, add more tests. + Fix value setting in CSR and dense conversions to Sparsity

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from 8873ee5 to 8a588b8 Compare October 1, 2019 08:56

Rename Sparsity to SparsityCsr.

b8bd477

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from 5ed0acc to b8bd477 Compare October 2, 2019 10:12

pratikvn requested a review from yhmtsai October 2, 2019 11:35

thoasm requested changes Oct 7, 2019

View reviewed changes

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from 91cf1e3 to f74bc3f Compare October 7, 2019 21:07

yhmtsai approved these changes Oct 8, 2019

View reviewed changes

core/matrix/csr.cpp Outdated Show resolved Hide resolved

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from f74bc3f to fa9fa9d Compare October 8, 2019 08:00

tcojean approved these changes Oct 8, 2019

View reviewed changes

core/matrix/dense.cpp Show resolved Hide resolved

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from fa9fa9d to c84768b Compare October 8, 2019 11:55

thoasm approved these changes Oct 8, 2019

View reviewed changes

Review update

745eb45

+ Update some forgotten names. + Improve some kernels. + Add some dense to sparsity csr conversion tests.

pratikvn force-pushed the sparsity-format-ref-omp-kernels branch from c84768b to 745eb45 Compare October 8, 2019 12:58

thoasm approved these changes Oct 8, 2019

View reviewed changes

pratikvn merged commit 976d71e into develop Oct 8, 2019

pratikvn deleted the sparsity-format-ref-omp-kernels branch October 8, 2019 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sparsity format Reference and OpenMP kernels #350

Sparsity format Reference and OpenMP kernels #350

pratikvn commented Sep 12, 2019 •

edited

Loading

codecov bot commented Sep 12, 2019 •

edited

Loading

yhmtsai commented Oct 1, 2019

pratikvn commented Oct 2, 2019

thoasm commented Oct 2, 2019

yhmtsai commented Oct 2, 2019

pratikvn commented Oct 2, 2019

yhmtsai commented Oct 2, 2019

pratikvn commented Oct 2, 2019

thoasm left a comment

yhmtsai left a comment

tcojean left a comment

thoasm left a comment

thoasm left a comment

Sparsity format Reference and OpenMP kernels #350

Sparsity format Reference and OpenMP kernels #350

Conversation

pratikvn commented Sep 12, 2019 • edited Loading

TODO

codecov bot commented Sep 12, 2019 • edited Loading

Codecov Report

yhmtsai commented Oct 1, 2019

pratikvn commented Oct 2, 2019

thoasm commented Oct 2, 2019

yhmtsai commented Oct 2, 2019

pratikvn commented Oct 2, 2019

yhmtsai commented Oct 2, 2019

pratikvn commented Oct 2, 2019

thoasm left a comment

Choose a reason for hiding this comment

yhmtsai left a comment

Choose a reason for hiding this comment

tcojean 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

pratikvn commented Sep 12, 2019 •

edited

Loading

codecov bot commented Sep 12, 2019 •

edited

Loading