From e500918e891ecdc1c0661338a5cdd1ee752e9826 Mon Sep 17 00:00:00 2001 From: Pratik Nayak Date: Thu, 1 Aug 2019 14:07:20 +0200 Subject: [PATCH] Some minor changes and clarifications. --- core/solver/trs.cpp | 5 ++ core/test/solver/trs.cpp | 4 +- dev_tools/scripts/todo_trs.txt | 53 ------------------- .../ginkgo/core/base/exception_helpers.hpp | 16 ++++++ include/ginkgo/core/solver/trs.hpp | 6 ++- omp/solver/trs_kernels.cpp | 6 ++- reference/solver/trs_kernels.cpp | 9 +++- reference/test/solver/trs.cpp | 19 +------ 8 files changed, 39 insertions(+), 79 deletions(-) delete mode 100644 dev_tools/scripts/todo_trs.txt diff --git a/core/solver/trs.cpp b/core/solver/trs.cpp index 023acb0d619..30034d967bd 100644 --- a/core/solver/trs.cpp +++ b/core/solver/trs.cpp @@ -70,6 +70,11 @@ void Trs::generate(const LinOp *system_matrix, using CsrMatrix = matrix::Csr; using Vector = matrix::Dense; GKO_ASSERT_IS_SQUARE_MATRIX(system_matrix); + // This is needed because it does not make sense to call the copy and + // convert if the existing matrix (if not CSR) is empty. + if (dynamic_cast(system_matrix) == nullptr) { + GKO_ASSERT_IS_NON_EMPTY_MATRIX(system_matrix); + } const auto exec = this->get_executor(); csr_system_matrix_ = copy_and_convert_to(exec, system_matrix); auto dense_b = as(b); diff --git a/core/test/solver/trs.cpp b/core/test/solver/trs.cpp index 40ae355dafb..4fa58fdc08a 100644 --- a/core/test/solver/trs.cpp +++ b/core/test/solver/trs.cpp @@ -33,8 +33,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include -#include - +#include #include @@ -56,7 +55,6 @@ class Trs : public ::testing::Test { std::shared_ptr exec; std::unique_ptr trs_factory; - std::unique_ptr solver; }; diff --git a/dev_tools/scripts/todo_trs.txt b/dev_tools/scripts/todo_trs.txt deleted file mode 100644 index 85dea742a12..00000000000 --- a/dev_tools/scripts/todo_trs.txt +++ /dev/null @@ -1,53 +0,0 @@ - -Summary: -Created solver file -core/solver/trs.cpp - This is where the trs algorithm needs to be implemented. - -Created class header -include/ginkgo/core/solver/trs.hpp - This is where the trs class functions need to be implemented. - -Created kernel header -core/solver/trs_kernels.hpp - This is where the algorithm-specific kernels need to be added. - -Created kernel file -reference/solver/trs_kernels.cpp - Reference kernels for trs need to be implemented here. - -Created kernel file -omp/solver/trs_kernels.cpp - OMP kernels for trs need to be implemented here. - -Created kernel file -cuda/solver/trs_kernels.cu - CUDA kernels for trs need to be implemented here. - -Created unit tests for trs -core/test/solver/trs.cpp - -Created unit tests for trs reference kernels -reference/test/solver/trs_kernels.cpp - -Created unit tests for trs OMP kernels -omp/test/solver/trs_kernels.cpp - -Created unit tests for trs CUDA kernels -cuda/test/solver/trs_kernels.cpp - -Modified core/CMakeLists.txt -Modified reference/CMakeLists.txt -Modified omp/CMakeLists.txt -Modified cuda/CMakeLists.txt -Modified core/test/solver/CMakeLists.txt -Modified reference/test/solver/CMakeLists.txt -Modified omp/test/solver/CMakeLists.txt -Modified cuda/test/solver/CMakeLists.txt -Modified core/device_hooks/common_kernels.inc.cpp -In all of the previous files cg was automatically replaced into trs. Ensure there is no inconsistency. - -All the imported code was commented and TODO items were generated in the new files. -Check all the modified files for '// TODO (script):' items -e.g. by using grep -HR '// TODO (script):' /home/pratik/Documents/00git/fork-ginkgo/dev_tools/scripts/../.. - diff --git a/include/ginkgo/core/base/exception_helpers.hpp b/include/ginkgo/core/base/exception_helpers.hpp index 3f50be4f79f..60167dde799 100644 --- a/include/ginkgo/core/base/exception_helpers.hpp +++ b/include/ginkgo/core/base/exception_helpers.hpp @@ -134,6 +134,22 @@ inline dim<2> get_size(const dim<2> &size) { return size; } ::gko::detail::get_size(_op1)[1], "expected square matrix"); \ } +/** + *Asserts that _op1 is a non-empty matrix. + * + *@throw DimensionMismatch if the number of rows of _op1 is different from the + * number of columns of _op1. + */ +#define GKO_ASSERT_IS_NON_EMPTY_MATRIX(_op1) \ + if (::gko::detail::get_size(_op1)[0] == 0 && \ + ::gko::detail::get_size(_op1)[1] == 0) { \ + throw ::gko::DimensionMismatch( \ + __FILE__, __LINE__, __func__, #_op1, \ + ::gko::detail::get_size(_op1)[0], \ + ::gko::detail::get_size(_op1)[1], #_op1, \ + ::gko::detail::get_size(_op1)[0], \ + ::gko::detail::get_size(_op1)[1], "expected non-empty matrix"); \ + } /** * Asserts that _op1 can be applied to _op2. diff --git a/include/ginkgo/core/solver/trs.hpp b/include/ginkgo/core/solver/trs.hpp index 80701cba705..90342094b13 100644 --- a/include/ginkgo/core/solver/trs.hpp +++ b/include/ginkgo/core/solver/trs.hpp @@ -227,7 +227,8 @@ class Trs : public EnableLinOp>, : parameters_{factory->get_parameters()}, EnableLinOp(factory->get_executor(), transpose(args.system_matrix->get_size())), - system_matrix_{std::move(args.system_matrix)} + system_matrix_{std::move(args.system_matrix)}, + b_{std::move(args.b)} { if (parameters_.preconditioner) { preconditioner_ = @@ -236,12 +237,13 @@ class Trs : public EnableLinOp>, preconditioner_ = matrix::Identity::create( this->get_executor(), this->get_size()[0]); } - this->generate(gko::lend(system_matrix_), gko::lend(args.b)); + this->generate(gko::lend(system_matrix_), gko::lend(b_)); } private: std::shared_ptr system_matrix_{}; + std::shared_ptr b_{}; std::shared_ptr> csr_system_matrix_{}; std::shared_ptr preconditioner_{}; diff --git a/omp/solver/trs_kernels.cpp b/omp/solver/trs_kernels.cpp index 50a7171bf06..f1478253767 100644 --- a/omp/solver/trs_kernels.cpp +++ b/omp/solver/trs_kernels.cpp @@ -56,7 +56,11 @@ template void generate(std::shared_ptr exec, const matrix::Csr *matrix, const matrix::Dense *b) -{} +{ + // This generate kernel is here to allow for a more sophisticated + // implementation as for the CUDA executor. This kernel would perform the + // "analysis" phase for the triangular matrix. +} template diff --git a/reference/solver/trs_kernels.cpp b/reference/solver/trs_kernels.cpp index 687d39bb0f3..22fe04b0976 100644 --- a/reference/solver/trs_kernels.cpp +++ b/reference/solver/trs_kernels.cpp @@ -38,7 +38,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include -#include + + namespace gko { namespace kernels { namespace reference { @@ -53,7 +54,11 @@ template void generate(std::shared_ptr exec, const matrix::Csr *matrix, const matrix::Dense *b) -{} +{ + // This generate kernel is here to allow for a more sophisticated + // implementation as for the CUDA executor. This kernel would perform the + // "analysis" phase for the triangular matrix. +} GKO_INSTANTIATE_FOR_EACH_VALUE_AND_INDEX_TYPE(GKO_DECLARE_TRS_GENERATE_KERNEL); diff --git a/reference/test/solver/trs.cpp b/reference/test/solver/trs.cpp index fb7bc596085..a6fd5be0557 100644 --- a/reference/test/solver/trs.cpp +++ b/reference/test/solver/trs.cpp @@ -32,6 +32,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include +#include #include @@ -41,10 +42,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include #include -#include -#include -#include -#include namespace { @@ -83,20 +80,6 @@ class Trs : public ::testing::Test { } } } - - static void assert_same_csr_matrices(const CsrMtx *m1, const CsrMtx *m2) - { - ASSERT_EQ(m1->get_size()[0], m2->get_size()[0]); - ASSERT_EQ(m1->get_size()[1], m2->get_size()[1]); - - for (gko::size_type i = 0; i < m1->get_size()[0] + 1; ++i) { - EXPECT_EQ(m1->get_const_row_ptrs()[i], m2->get_const_row_ptrs()[i]); - } - for (gko::size_type i = 0; i < m1->get_num_stored_elements(); ++i) { - EXPECT_EQ(m1->get_const_col_idxs()[i], m2->get_const_col_idxs()[i]); - EXPECT_EQ(m1->get_const_values()[i], m2->get_const_values()[i]); - } - } };