Review update and additional fixes

- GKO_NOT_SUPPORTED is the only macro that does not include the `throw`. This was not only a problem in this code, but also in unrelated code, which is also fixed within this commit - An Empty Dense matrix can now be converted to a CSR matrix - Moved the `GKO_NOT_SUPPORTED` to the cuda kernel, so empty matrices do not throw when applying a strategy.
ginkgo-project · tcojean · Dec 2, 2019 · Oct 21, 2019 · Oct 29, 2019 · Oct 30, 2019
commit a942a897b1e1e55b5d3c31ebe9c0aa02f96db2ee
diff --git a/core/matrix/dense.cpp b/core/matrix/dense.cpp
@@ -108,12 +108,21 @@ inline void conversion_helper(Csr<ValueType, IndexType> *result,
 {
     auto exec = source->get_executor();
 
-    size_type num_stored_nonzeros = 0;
-    exec->run(dense::make_count_nonzeros(source, &num_stored_nonzeros));
-    auto tmp = Csr<ValueType, IndexType>::create(
-        exec, source->get_size(), num_stored_nonzeros, result->get_strategy());
-    exec->run(op(tmp.get(), source));
-    tmp->move_to(result);
+    if (source->get_size()) {
+        size_type num_stored_nonzeros = 0;
+        exec->run(dense::make_count_nonzeros(source, &num_stored_nonzeros));
+        auto tmp = Csr<ValueType, IndexType>::create(exec, source->get_size(),
+                                                     num_stored_nonzeros,
+                                                     result->get_strategy());
+        exec->run(op(tmp.get(), source));
+        tmp->move_to(result);
+    }
+    // If source is empty, there is no need to copy data or to call kernels
+    else {
+        auto tmp =
+            Csr<ValueType, IndexType>::create(exec, result->get_strategy());
+        tmp->move_to(result);
+    }
 }
 
 

diff --git a/core/stop/residual_norm_reduction.cpp b/core/stop/residual_norm_reduction.cpp
@@ -63,7 +63,7 @@ bool ResidualNormReduction<ValueType>::check_impl(
         dense_r->compute_norm2(u_dense_tau_.get());
         dense_tau = u_dense_tau_.get();
     } else {
-        GKO_NOT_SUPPORTED(nullptr);
+        throw GKO_NOT_SUPPORTED(nullptr);
     }
     bool all_converged = true;
 

diff --git a/cuda/matrix/csr_kernels.cu b/cuda/matrix/csr_kernels.cu
@@ -673,6 +673,8 @@ void spmv(std::shared_ptr<const CudaExecutor> exec,
                 as_cuda_type(b->get_const_values()),
                 as_cuda_type(b->get_stride()), as_cuda_type(c->get_values()),
                 as_cuda_type(c->get_stride()));
+        } else {
+            throw GKO_NOT_SUPPORTED(nwarps);
         }
     } else if (a->get_strategy()->get_name() == "merge_path") {
         int items_per_thread =

diff --git a/include/ginkgo/core/matrix/csr.hpp b/include/ginkgo/core/matrix/csr.hpp
@@ -225,8 +225,6 @@ class Csr : public EnableLinOp<Csr<ValueType, IndexType>>,
                 if (!is_srow_on_host) {
                     *mtx_srow = srow_host;
                 }
-            } else {
-                GKO_NOT_SUPPORTED(nwarps);
             }
         }
 

diff --git a/include/ginkgo/core/preconditioner/jacobi.hpp b/include/ginkgo/core/preconditioner/jacobi.hpp
@@ -489,9 +489,8 @@ class Jacobi : public EnableLinOp<Jacobi<ValueType, IndexType>>,
                       parameters_.block_pointers.get_num_elems() - 1)),
           conditioning_(factory->get_executor())
     {
-        if (parameters_.max_block_size >= 32 ||
-            parameters_.max_block_size < 1) {
-            GKO_NOT_SUPPORTED(this);
+        if (parameters_.max_block_size > 32 || parameters_.max_block_size < 1) {
+            throw GKO_NOT_SUPPORTED(this);
         }
         parameters_.block_pointers.set_executor(this->get_executor());
         parameters_.storage_optimization.block_wise.set_executor(

diff --git a/include/ginkgo/core/stop/combined.hpp b/include/ginkgo/core/stop/combined.hpp
@@ -117,7 +117,7 @@ std::shared_ptr<const CriterionFactory> combine(FactoryContainer &&factories)
 {
     switch (factories.size()) {
     case 0:
-        GKO_NOT_SUPPORTED(nullptr);
+        throw GKO_NOT_SUPPORTED(nullptr);
         return nullptr;
     case 1:
         return factories[0];

diff --git a/include/ginkgo/core/stop/residual_norm_reduction.hpp b/include/ginkgo/core/stop/residual_norm_reduction.hpp
@@ -100,7 +100,7 @@ class ResidualNormReduction
           device_storage_{factory->get_executor(), 2}
     {
         if (args.initial_residual == nullptr) {
-            GKO_NOT_SUPPORTED(nullptr);
+            throw GKO_NOT_SUPPORTED(nullptr);
         }
 
         auto exec = factory->get_executor();

diff --git a/reference/test/matrix/csr_kernels.cpp b/reference/test/matrix/csr_kernels.cpp
@@ -343,6 +343,19 @@ class Csr : public ::testing::Test {
 };
 
 
+TEST_F(Csr, CopyEmptyMatrix)
+{
+    auto strategy = std::make_shared<Mtx::load_balance>(0);
+    auto from_mtx = gko::matrix::Dense<>::create(exec, gko::dim<2>(0, 0));
+    auto to_mtx =
+        gko::matrix::Csr<>::create(exec, gko::dim<2>{0, 0}, 0, strategy);
+
+    from_mtx->convert_to(to_mtx.get());
+
+    ASSERT_FALSE(to_mtx->get_size());
+}
+
+
 TEST_F(Csr, AppliesToDenseVector)
 {
     auto x = gko::initialize<Vec>({2.0, 1.0, 4.0}, exec);