Add diagonal matrix format for diagonal scaling #580

fritzgoebel · 2020-07-01T13:47:47Z

For now, Diagonal can be applied to Dense and Csr.
The normal apply does row wise scaling, the right apply does column wise scaling.

yhmtsai · 2020-07-07T07:22:36Z

Does this pr should be merged first? After this, #563 extract_diagonal will return Diagonal not Dense

pratikvn · 2020-07-13T09:17:40Z

I think we came to a conclusion to merge this first and make the extract diagonal return a Diagonal matrix format ?

yhmtsai

first part of my review

core/matrix/diagonal.cpp

cuda/test/matrix/dense_kernels.cpp

cuda/test/matrix/diagonal_kernels.cpp

hip/test/matrix/dense_kernels.hip.cpp

hip/test/matrix/diagonal_kernels.cpp

include/ginkgo/core/matrix/diagonal.hpp

codecov · 2020-07-27T15:09:01Z

Codecov Report

Merging #580 into develop will decrease coverage by 0.15%.
The diff coverage is 85.84%.

@@             Coverage Diff             @@
##           develop     #580      +/-   ##
===========================================
- Coverage    93.01%   92.85%   -0.16%     
===========================================
  Files          296      303       +7     
  Lines        20660    21105     +445     
===========================================
+ Hits         19216    19598     +382     
- Misses        1444     1507      +63

Impacted Files	Coverage Δ
core/device_hooks/common_kernels.inc.cpp	`0.00% <0.00%> (ø)`
core/factorization/par_ict.cpp	`100.00% <ø> (ø)`
include/ginkgo/core/base/exception_helpers.hpp	`90.90% <ø> (ø)`
include/ginkgo/core/matrix/dense.hpp	`97.70% <ø> (ø)`
core/matrix/diagonal.cpp	`64.94% <64.94%> (ø)`
reference/test/matrix/diagonal_kernels.cpp	`83.52% <83.52%> (ø)`
omp/test/matrix/dense_kernels.cpp	`99.66% <92.30%> (-0.34%)`	⬇️
core/matrix/dense.cpp	`99.19% <100.00%> (+0.01%)`	⬆️
core/test/matrix/diagonal.cpp	`100.00% <100.00%> (ø)`
include/ginkgo/core/matrix/diagonal.hpp	`100.00% <100.00%> (ø)`
... and 13 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 ef7a837...07a58a2. Read the comment docs.

yhmtsai

LGTM. need some minor changes

yhmtsai · 2020-08-03T13:28:43Z

core/matrix/diagonal.cpp

+ Csr<ValueType, IndexType> *result) const
+{
+ auto exec = this->get_executor();
+ auto tmp = Csr<ValueType, IndexType>::create(exec, this->get_size(),


need to set the same strategy of result csr.

omp/matrix/diagonal_kernels.cpp

yhmtsai · 2020-08-03T13:43:42Z

reference/test/matrix/diagonal_kernels.cpp

+}
+
+
+TYPED_TEST(Diagonal, ConverstToCsr)


Suggested change

TYPED_TEST(Diagonal, ConverstToCsr)

TYPED_TEST(Diagonal, ConvertsToCsr)

pratikvn

Only a minor nit

pratikvn · 2020-08-11T14:18:21Z

core/matrix/diagonal.cpp

+inline void read_impl(MatrixType *mtx, const MatrixData &data)
+{
+ // Diagonal matrices are assumed to be square.
+ GKO_ASSERT_EQ(data.size[0], data.size[1]);


I think we have a GKO_ASSERT_IS_SQUARE_MATRIX which throws DimensionMismatch which is better suited here. But maybe you have to add a get_size() method to MatrixData.

…ctor

sonarcloud · 2020-08-13T03:48:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
39 Code Smells

40.2% Coverage
2.4% Duplication

The version of Java (1.8.0_121) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

Release 1.3.0 of Ginkgo. The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.3.0. This release brings CUDA 11 support, changes the default C++ standard to be C++14 instead of C++11, adds a new Diagonal matrix format and capacity for diagonal extraction, significantly improves the CMake configuration output format, adds the Ginkgo paper which got accepted into the Journal of Open Source Software (JOSS), and fixes multiple issues. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, all versions after 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + HIP module: ROCm 2.8+ + Windows + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, all versions after 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: + Add paper for Journal of Open Source Software (JOSS). [#479](#479) + Add a DiagonalExtractable interface. [#563](#563) + Add a new diagonal Matrix Format. [#580](#580) + Add Cuda11 support. [#603](#603) + Add information output after CMake configuration. [#610](#610) + Add a new preconditioner export example. [#595](#595) + Add a new cuda-memcheck CI job. [#592](#592) Changes: + Use unified memory in CUDA debug builds. [#621](#621) + Improve `BENCHMARKING.md` with more detailed info. [#619](#619) + Use C++14 standard instead of C++11. [#611](#611) + Update the Ampere sm information and CudaArchitectureSelector. [#588](#588) Fixes: + Fix documentation warnings and errors. [#624](#624) + Fix warnings for diagonal matrix format. [#622](#622) + Fix criterion factory parameters in CUDA. [#586](#586) + Fix the norm-type in the examples. [#612](#612) + Fix the WAW race in OpenMP is_sorted_by_column_index. [#617](#617) + Fix the example's exec_map by creating the executor only if requested. [#602](#602) + Fix some CMake warnings. [#614](#614) + Fix Windows building documentation. [#601](#601) + Warn when CXX and CUDA host compiler do not match. [#607](#607) + Fix reduce_add, prefix_sum, and doc-build. [#593](#593) + Fix find_library(cublas) issue on machines installing multiple cuda. [#591](#591) + Fix allocator in sellp read. [#589](#589) + Fix the CAS with HIP and NVIDIA backends. [#585](#585) Deletions: + Remove unused preconditioner parameter in LowerTrs. [#587](#587) Related PR: #625

The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.3.0. This release brings CUDA 11 support, changes the default C++ standard to be C++14 instead of C++11, adds a new Diagonal matrix format and capacity for diagonal extraction, significantly improves the CMake configuration output format, adds the Ginkgo paper which got accepted into the Journal of Open Source Software (JOSS), and fixes multiple issues. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, all versions after 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + HIP module: ROCm 2.8+ + Windows + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, all versions after 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: + Add paper for Journal of Open Source Software (JOSS). [#479](#479) + Add a DiagonalExtractable interface. [#563](#563) + Add a new diagonal Matrix Format. [#580](#580) + Add Cuda11 support. [#603](#603) + Add information output after CMake configuration. [#610](#610) + Add a new preconditioner export example. [#595](#595) + Add a new cuda-memcheck CI job. [#592](#592) Changes: + Use unified memory in CUDA debug builds. [#621](#621) + Improve `BENCHMARKING.md` with more detailed info. [#619](#619) + Use C++14 standard instead of C++11. [#611](#611) + Update the Ampere sm information and CudaArchitectureSelector. [#588](#588) Fixes: + Fix documentation warnings and errors. [#624](#624) + Fix warnings for diagonal matrix format. [#622](#622) + Fix criterion factory parameters in CUDA. [#586](#586) + Fix the norm-type in the examples. [#612](#612) + Fix the WAW race in OpenMP is_sorted_by_column_index. [#617](#617) + Fix the example's exec_map by creating the executor only if requested. [#602](#602) + Fix some CMake warnings. [#614](#614) + Fix Windows building documentation. [#601](#601) + Warn when CXX and CUDA host compiler do not match. [#607](#607) + Fix reduce_add, prefix_sum, and doc-build. [#593](#593) + Fix find_library(cublas) issue on machines installing multiple cuda. [#591](#591) + Fix allocator in sellp read. [#589](#589) + Fix the CAS with HIP and NVIDIA backends. [#585](#585) Deletions: + Remove unused preconditioner parameter in LowerTrs. [#587](#587) Related PR: #627

fritzgoebel requested review from upsj, pratikvn, thoasm, yhmtsai, hartwiganzt and tcojean July 1, 2020 13:47

fritzgoebel self-assigned this Jul 1, 2020

fritzgoebel force-pushed the add_diagonal_scaling branch from 65e2090 to 8d364be Compare July 17, 2020 15:11

fritzgoebel removed the 1:ST:WIP This PR is a work in progress. Not ready for review. label Jul 17, 2020

fritzgoebel force-pushed the add_diagonal_scaling branch from 8d364be to ffa2882 Compare July 17, 2020 15:28

yhmtsai requested changes Jul 22, 2020

View reviewed changes

fritzgoebel force-pushed the add_diagonal_scaling branch 2 times, most recently from bd699ea to 31d99e8 Compare July 27, 2020 09:20

pratikvn added the 1:ST:ready-for-review This PR is ready for review label Jul 31, 2020

yhmtsai approved these changes Aug 3, 2020

View reviewed changes

tcojean mentioned this pull request Aug 5, 2020

Add extract_diagonal for all matrix formats #563

Merged

2 tasks

tcojean removed mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. labels Aug 6, 2020

pratikvn approved these changes Aug 11, 2020

View reviewed changes

fritzgoebel force-pushed the add_diagonal_scaling branch from b495dc4 to 00238fc Compare August 12, 2020 07:38

fritzgoebel added 19 commits August 12, 2020 16:23

Reference implementation of apply_to_dense

1c17716

Add apply_to_dense and right_apply_to_dense for omp, cuda and hip

4f860aa

reference implementation of apply_to_csr

3c5a21c

Add reference tests for apply_to_csr

ea0e66d

add (r)apply_to_csr for omp, cuda and hip

381e7d7

GKO_NOT_IMPLEMENTED for not yet supported matrix formats

efaaf09

Add_scaled_diag for dense

defaf16

Add tests for CUDA and HIP

752ac1b

Add missing includes and fix typos

8a1ea39

Add convert_to_csr to diagonal matrix format

ef876d5

fix typos

78508ed

Additional constructor for creating a diagonal matrix from a dense ve…

e33a11b

…ctor

handle dimension mismatch for right apply

0c50b38

add tests for dimension mismatch

e702ca6

Updates from code review

451e597

Fix Hip test file name

3751a75

Suggestions from code review

3b6d171

Remove IndexType template from diagonal matrix format

39bffc8

Suggestions from code review

07a58a2

fritzgoebel force-pushed the add_diagonal_scaling branch from 00238fc to 07a58a2 Compare August 12, 2020 14:24

fritzgoebel merged commit bb62766 into develop Aug 13, 2020

fritzgoebel deleted the add_diagonal_scaling branch August 13, 2020 07:28

upsj 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 Aug 13, 2020

upsj mentioned this pull request May 5, 2021

Adding Functionality needed by openCARP #555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add diagonal matrix format for diagonal scaling #580

Add diagonal matrix format for diagonal scaling #580

fritzgoebel commented Jul 1, 2020 •

edited

Loading

yhmtsai commented Jul 7, 2020

pratikvn commented Jul 13, 2020

yhmtsai left a comment

codecov bot commented Jul 27, 2020 •

edited

Loading

yhmtsai left a comment

yhmtsai Aug 3, 2020

yhmtsai Aug 3, 2020

pratikvn left a comment

pratikvn Aug 11, 2020

sonarcloud bot commented Aug 13, 2020

	TYPED_TEST(Diagonal, ConverstToCsr)
	TYPED_TEST(Diagonal, ConvertsToCsr)

Add diagonal matrix format for diagonal scaling #580

Add diagonal matrix format for diagonal scaling #580

Conversation

fritzgoebel commented Jul 1, 2020 • edited Loading

yhmtsai commented Jul 7, 2020

pratikvn commented Jul 13, 2020

yhmtsai left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 27, 2020 • edited Loading

Codecov Report

yhmtsai left a comment

Choose a reason for hiding this comment

yhmtsai Aug 3, 2020

Choose a reason for hiding this comment

yhmtsai Aug 3, 2020

Choose a reason for hiding this comment

pratikvn left a comment

Choose a reason for hiding this comment

pratikvn Aug 11, 2020

Choose a reason for hiding this comment

sonarcloud bot commented Aug 13, 2020

fritzgoebel commented Jul 1, 2020 •

edited

Loading

codecov bot commented Jul 27, 2020 •

edited

Loading