Some minor changes and clarifications.

ginkgo-project · Aug 10, 2019 · e500918 · e500918
1 parent 7f9e520
commit e500918
Show file tree

Hide file tree

Showing 8 changed files with 39 additions and 79 deletions.
diff --git a/core/solver/trs.cpp b/core/solver/trs.cpp
@@ -70,6 +70,11 @@ void Trs<ValueType, IndexType>::generate(const LinOp *system_matrix,
  using CsrMatrix = matrix::Csr<ValueType, IndexType>;
  using Vector = matrix::Dense<ValueType>;
  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<const CsrMatrix *>(system_matrix) == nullptr) {
+ GKO_ASSERT_IS_NON_EMPTY_MATRIX(system_matrix);
+ }
  const auto exec = this->get_executor();
  csr_system_matrix_ = copy_and_convert_to<CsrMatrix>(exec, system_matrix);
  auto dense_b = as<const Vector>(b);

diff --git 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 <ginkgo/core/solver/trs.hpp>
 
 
-#include <typeinfo>
-
+#include <memory>
 
 #include <gtest/gtest.h>
 
@@ -56,7 +55,6 @@ class Trs : public ::testing::Test {
 
  std::shared_ptr<const gko::Executor> exec;
  std::unique_ptr<Solver::Factory> trs_factory;
- std::unique_ptr<gko::LinOp> solver;
 };
 
 

diff --git a/dev_tools/scripts/todo_trs.txt b/dev_tools/scripts/todo_trs.txt
diff --git 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
@@ -227,7 +227,8 @@ class Trs : public EnableLinOp<Trs<ValueType, IndexType>>,
  : parameters_{factory->get_parameters()},
  EnableLinOp<Trs>(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<Trs<ValueType, IndexType>>,
  preconditioner_ = matrix::Identity<ValueType>::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<const LinOp> system_matrix_{};
+ std::shared_ptr<const LinOp> b_{};
  std::shared_ptr<const matrix::Csr<ValueType, IndexType>>
  csr_system_matrix_{};
  std::shared_ptr<const LinOp> preconditioner_{};

diff --git a/omp/solver/trs_kernels.cpp b/omp/solver/trs_kernels.cpp
@@ -56,7 +56,11 @@ template <typename ValueType, typename IndexType>
 void generate(std::shared_ptr<const OmpExecutor> exec,
  const matrix::Csr<ValueType, IndexType> *matrix,
  const matrix::Dense<ValueType> *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 <typename ValueType, typename IndexType>

diff --git 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 <ginkgo/core/base/math.hpp>
 #include <ginkgo/core/base/types.hpp>
 #include <ginkgo/core/matrix/csr.hpp>
-#include <iostream>
+
+
 namespace gko {
 namespace kernels {
 namespace reference {
@@ -53,7 +54,11 @@ template <typename ValueType, typename IndexType>
 void generate(std::shared_ptr<const ReferenceExecutor> exec,
  const matrix::Csr<ValueType, IndexType> *matrix,
  const matrix::Dense<ValueType> *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
@@ -32,6 +32,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include <ginkgo/core/solver/trs.hpp>
 
+#include <memory>
 
 #include <gtest/gtest.h>
 
@@ -41,10 +42,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <ginkgo/core/base/executor.hpp>
 #include <ginkgo/core/matrix/csr.hpp>
 #include <ginkgo/core/matrix/dense.hpp>
-#include <ginkgo/core/stop/combined.hpp>
-#include <ginkgo/core/stop/iteration.hpp>
-#include <ginkgo/core/stop/residual_norm_reduction.hpp>
-#include <ginkgo/core/stop/time.hpp>
 
 
 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]);
- }
- }
 };