Address Review comments

core/CMakeLists.txt

@@ -11,7 +11,6 @@ target_sources(ginkgo
     base/index_set.cpp
     base/mtx_io.cpp
     base/perturbation.cpp
-    base/reordered.cpp
     base/version.cpp
     distributed/partition.cpp
     factorization/elimination_forest.cpp
@@ -44,6 +43,7 @@ target_sources(ginkgo
     preconditioner/isai.cpp
     preconditioner/jacobi.cpp
     reorder/rcm.cpp
+    reorder/scaled_reordered.cpp
     solver/bicg.cpp
     solver/bicgstab.cpp
     solver/cb_gmres.cpp

core/base/reordered.cpp → core/reorder/scaled_reordered.cpp

@@ -30,7 +30,10 @@ THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 ******************************<GINKGO LICENSE>*******************************/
 
-#include <ginkgo/core/base/reordered.hpp>
+#include <ginkgo/core/reorder/scaled_reordered.hpp>
+
+
+#include <utility>
 
 
 #include <ginkgo/core/base/precision_dispatch.hpp>
@@ -62,15 +65,12 @@ void ScaledReordered<ValueType, IndexType>::apply_impl(const LinOp* b,
                                             cache_.intermediate.get());
                 std::swap(cache_.inner_x, cache_.intermediate);
             }
-            if (permutation_) {
-                auto permutation_array =
-                    make_array_view(exec, permutation_->get_size()[0],
-                                    permutation_->get_permutation());
-                cache_.inner_b->row_permute(&permutation_array,
+            if (permutation_array_) {
+                cache_.inner_b->row_permute(permutation_array_.get(),
                                             cache_.intermediate.get());
                 std::swap(cache_.inner_b, cache_.intermediate);
                 if (inner_operator_->apply_uses_initial_guess()) {
-                    cache_.inner_x->row_permute(&permutation_array,
+                    cache_.inner_x->row_permute(permutation_array_.get(),
                                                 cache_.intermediate.get());
                     std::swap(cache_.inner_x, cache_.intermediate);
                 }
@@ -79,11 +79,8 @@ void ScaledReordered<ValueType, IndexType>::apply_impl(const LinOp* b,
             inner_operator_->apply(cache_.inner_b.get(), cache_.inner_x.get());
 
             // Permute and scale the solution vector back.
-            if (permutation_) {
-                auto permutation_array =
-                    make_array_view(exec, permutation_->get_size()[0],
-                                    permutation_->get_permutation());
-                cache_.inner_x->inverse_row_permute(&permutation_array,
+            if (permutation_array_) {
+                cache_.inner_x->inverse_row_permute(permutation_array_.get(),
                                                     cache_.intermediate.get());
                 std::swap(cache_.inner_x, cache_.intermediate);
             }

core/test/base/CMakeLists.txt

@@ -19,7 +19,6 @@ ginkgo_create_test(polymorphic_object)
 ginkgo_create_test(range)
 ginkgo_create_test(range_accessors)
 ginkgo_create_test(sanitizers ADDITIONAL_LIBRARIES Threads::Threads)
-ginkgo_create_test(reordered)
 ginkgo_create_test(types)
 ginkgo_create_test(utils)
 ginkgo_create_test(version)

core/test/reorder/CMakeLists.txt

@@ -1 +1,2 @@
 ginkgo_create_test(rcm)
+ginkgo_create_test(scaled_reordered)

core/test/base/reordered.cpp → core/test/reorder/scaled_reordered.cpp

@@ -30,7 +30,7 @@ THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 ******************************<GINKGO LICENSE>*******************************/
 
-#include <ginkgo/core/base/reordered.hpp>
+#include <ginkgo/core/reorder/scaled_reordered.hpp>
 
 
 #include <memory>

include/ginkgo/core/reorder/rcm.hpp

@@ -92,10 +92,10 @@ enum class starting_strategy { minimum_degree, pseudo_peripheral };
  * @ingroup reorder
  */
 template <typename ValueType = default_precision, typename IndexType = int32>
-class Rcm
-    : public EnablePolymorphicObject<Rcm<ValueType, IndexType>, ReorderingBase>,
-      public EnablePolymorphicAssignment<Rcm<ValueType, IndexType>> {
-    friend class EnablePolymorphicObject<Rcm, ReorderingBase>;
+class Rcm : public EnablePolymorphicObject<Rcm<ValueType, IndexType>,
+                                           ReorderingBase<IndexType>>,
+            public EnablePolymorphicAssignment<Rcm<ValueType, IndexType>> {
+    friend class EnablePolymorphicObject<Rcm, ReorderingBase<IndexType>>;
 
 public:
     using SparsityMatrix = matrix::SparsityCsr<ValueType, IndexType>;
@@ -109,7 +109,7 @@ class Rcm
      *
      * @return the permutation (permutation matrix)
      */
-    std::shared_ptr<const LinOp> get_permutation() const override
+    std::shared_ptr<const PermutationMatrix> get_permutation() const
     {
         return permutation_;
     }
@@ -120,11 +120,20 @@ class Rcm
      *
      * @return the inverse permutation (permutation matrix)
      */
-    std::shared_ptr<const LinOp> get_inverse_permutation() const override
+    std::shared_ptr<const PermutationMatrix> get_inverse_permutation() const
     {
         return inv_permutation_;
     }
 
+    std::shared_ptr<const array<index_type>> get_permutation_array()
+        const override
+    {
+        auto permutation_array =
+            make_array_view(this->get_executor(), permutation_->get_size()[0],
+                            permutation_->get_permutation());
+        return std::make_shared<const array<index_type>>(permutation_array);
+    }
+
     GKO_CREATE_FACTORY_PARAMETERS(parameters, Factory)
     {
         /**
@@ -152,11 +161,13 @@ class Rcm
                   std::unique_ptr<SparsityMatrix> adjacency_matrix) const;
 
     explicit Rcm(std::shared_ptr<const Executor> exec)
-        : EnablePolymorphicObject<Rcm, ReorderingBase>(std::move(exec))
+        : EnablePolymorphicObject<Rcm, ReorderingBase<IndexType>>(
+              std::move(exec))
     {}
 
     explicit Rcm(const Factory* factory, const ReorderingBaseArgs& args)
-        : EnablePolymorphicObject<Rcm, ReorderingBase>(factory->get_executor()),
+        : EnablePolymorphicObject<Rcm, ReorderingBase<IndexType>>(
+              factory->get_executor()),
           parameters_{factory->get_parameters()}
     {
         // Always execute the reordering on the cpu.

include/ginkgo/core/reorder/reordering_base.hpp

@@ -59,11 +59,13 @@ namespace reorder {
  * It contains a factory to instantiate the reorderings. It is up to each
  * specific reordering to decide what to do with the data that is passed to it.
  */
-class ReorderingBase : public EnableAbstractPolymorphicObject<ReorderingBase> {
+template <typename IndexType = int32>
+class ReorderingBase
+    : public EnableAbstractPolymorphicObject<ReorderingBase<IndexType>> {
 public:
-    virtual std::shared_ptr<const gko::LinOp> get_permutation() const = 0;
+    using index_type = IndexType;
 
-    virtual std::shared_ptr<const gko::LinOp> get_inverse_permutation()
+    virtual std::shared_ptr<const array<index_type>> get_permutation_array()
         const = 0;
 
 protected:
@@ -87,36 +89,6 @@ struct ReorderingBaseArgs {
 };
 
 
-/**
- * Declares an Abstract Factory specialized for ReorderingBases
- */
-using ReorderingBaseFactory =
-    AbstractFactory<ReorderingBase, ReorderingBaseArgs>;
-
-
-/**
- * This is an alias for the EnableDefaultFactory mixin, which correctly sets the
- * template parameters to enable a subclass of ReorderingBaseFactory.
- *
- * @tparam ConcreteFactory  the concrete factory which is being implemented
- *                          [CRTP parmeter]
- * @tparam ConcreteReorderingBase  the concrete ReorderingBase type which this
- * factory produces, needs to have a constructor which takes a const
- * ConcreteFactory *, and a const ReorderingBaseArgs * as parameters.
- * @tparam ParametersType  a subclass of enable_parameters_type template which
- *                         defines all of the parameters of the factory
- * @tparam PolymorphicBase  parent of ConcreteFactory in the polymorphic
- *                          hierarchy, has to be a subclass of
- * ReorderingBaseFactory
- */
-template <typename ConcreteFactory, typename ConcreteReorderingBase,
-          typename ParametersType,
-          typename PolymorphicBase = ReorderingBaseFactory>
-using EnableDefaultReorderingBaseFactory =
-    EnableDefaultFactory<ConcreteFactory, ConcreteReorderingBase,
-                         ParametersType, PolymorphicBase>;
-
-
 /**
  * This macro will generate a default implementation of a ReorderingBaseFactory
  * for the ReorderingBase subclass it is defined in.
@@ -144,26 +116,37 @@ public:                                                                        \
     }                                                                          \
                                                                                \
     class _factory_name                                                        \
-        : public ::gko::reorder::EnableDefaultReorderingBaseFactory<           \
-              _factory_name, _reordering_base, _parameters_name##_type> {      \
+        : public ::gko::EnableDefaultFactory<                                  \
+              _factory_name, _reordering_base, _parameters_name##_type,        \
+              ::gko::AbstractFactory<ReorderingBase<IndexType>,                \
+                                     ::gko::reorder::ReorderingBaseArgs>> {    \
         friend class ::gko::EnablePolymorphicObject<                           \
-            _factory_name, ::gko::reorder::ReorderingBaseFactory>;             \
+            _factory_name,                                                     \
+            ::gko::AbstractFactory<::gko::reorder::ReorderingBase<IndexType>,  \
+                                   ::gko::reorder::ReorderingBaseArgs>>;       \
         friend class ::gko::enable_parameters_type<_parameters_name##_type,    \
                                                    _factory_name>;             \
         explicit _factory_name(std::shared_ptr<const ::gko::Executor> exec)    \
-            : ::gko::reorder::EnableDefaultReorderingBaseFactory<              \
-                  _factory_name, _reordering_base, _parameters_name##_type>(   \
-                  std::move(exec))                                             \
+            : ::gko::EnableDefaultFactory<                                     \
+                  _factory_name, _reordering_base, _parameters_name##_type,    \
+                  ::gko::AbstractFactory<                                      \
+                      ::gko::reorder::ReorderingBase<IndexType>,               \
+                      ::gko::reorder::ReorderingBaseArgs>>(std::move(exec))    \
         {}                                                                     \
         explicit _factory_name(std::shared_ptr<const ::gko::Executor> exec,    \
                                const _parameters_name##_type& parameters)      \
-            : ::gko::reorder::EnableDefaultReorderingBaseFactory<              \
-                  _factory_name, _reordering_base, _parameters_name##_type>(   \
-                  std::move(exec), parameters)                                 \
+            : ::gko::EnableDefaultFactory<                                     \
+                  _factory_name, _reordering_base, _parameters_name##_type,    \
+                  ::gko::AbstractFactory<                                      \
+                      ::gko::reorder::ReorderingBase<IndexType>,               \
+                      ::gko::reorder::ReorderingBaseArgs>>(std::move(exec),    \
+                                                           parameters)         \
         {}                                                                     \
     };                                                                         \
-    friend ::gko::reorder::EnableDefaultReorderingBaseFactory<                 \
-        _factory_name, _reordering_base, _parameters_name##_type>;             \
+    friend ::gko::EnableDefaultFactory<                                        \
+        _factory_name, _reordering_base, _parameters_name##_type,              \
+        ::gko::AbstractFactory<::gko::reorder::ReorderingBase<IndexType>,      \
+                               ::gko::reorder::ReorderingBaseArgs>>;           \
                                                                                \
 private:                                                                       \
     _parameters_name##_type _parameters_name##_;                               \

include/ginkgo/core/base/reordered.hpp → include/ginkgo/core/reorder/scaled_reordered.hpp

@@ -30,8 +30,8 @@ THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 ******************************<GINKGO LICENSE>*******************************/
 
-#ifndef GKO_PUBLIC_CORE_BASE_REORDERED_HPP_
-#define GKO_PUBLIC_CORE_BASE_REORDERED_HPP_
+#ifndef GKO_PUBLIC_CORE_BASE_SCALED_REORDERED_HPP_
+#define GKO_PUBLIC_CORE_BASE_SCALEDREORDERED_HPP_
 
 
 #include <ginkgo/core/base/abstract_factory.hpp>
@@ -57,14 +57,14 @@ namespace gko {
  * stability by reducing the condition number of the system matrix.
  *
  * With a permutation matrix P, a row scaling R and a column scaling C, the
- * inner operator is applied to the system matrix P*R*A*C*P^T instead of A,
- * so the instead of A*x = b the inner operator attempts to solve the equivalent
+ * inner operator is applied to the system matrix P*R*A*C*P^T instead of A.
+ * Instead of A*x = b, the inner operator attempts to solve the equivalent
  * linear system P*R*A*C*P^T*y = P*R*b and retrieves the solution x = C*P^T*y.
  * Note: The inner system matrix is computed from a clone of A, so the original
  * system matrix is not changed.
  *
  * @tparam ValueType  Type of the values of all matrices used in this class
- * @tparam IndexType  Type of the indices of all matrix used in this class
+ * @tparam IndexType  Type of the indices of all matrices used in this class
  */
 template <typename ValueType = default_precision, typename IndexType = int32>
 class ScaledReordered
@@ -75,6 +75,9 @@ class ScaledReordered
 public:
     using value_type = ValueType;
     using index_type = IndexType;
+    using ReorderingBaseFactory =
+        AbstractFactory<reorder::ReorderingBase<IndexType>,
+                        reorder::ReorderingBaseArgs>;
 
     std::shared_ptr<const LinOp> get_system_matrix() const
     {
@@ -98,7 +101,7 @@ class ScaledReordered
         /**
          * The reordering that is to be applied to the system matrix.
          */
-        std::shared_ptr<const reorder::ReorderingBaseFactory>
+        std::shared_ptr<const ReorderingBaseFactory>
             GKO_FACTORY_PARAMETER_SCALAR(reordering, nullptr);
 
         /**
@@ -155,14 +158,9 @@ class ScaledReordered
         // permute the system matrix accordingly.
         if (parameters_.reordering) {
             auto reordering = parameters_.reordering->generate(system_matrix_);
-            auto perm = reordering->get_permutation();
-            permutation_ =
-                clone(exec, as<matrix::Permutation<index_type>>(perm));
-            auto permutation_array =
-                make_array_view(exec, permutation_->get_size()[0],
-                                permutation_->get_permutation());
+            permutation_array_ = reordering->get_permutation_array();
             system_matrix_ = as<Permutable<index_type>>(system_matrix_)
-                                 ->permute(&permutation_array);
+                                 ->permute(permutation_array_.get());
         }
 
         // Generate the inner operator with the scaled and reordered system
@@ -193,7 +191,8 @@ class ScaledReordered
      */
     void set_cache_to(const LinOp* b, const LinOp* x) const
     {
-        if (cache_.inner_b == nullptr) {
+        if (cache_.inner_b == nullptr ||
+            cache_.inner_b->get_size() != b->get_size()) {
             const auto size = b->get_size();
             cache_.inner_b =
                 matrix::Dense<value_type>::create(this->get_executor(), size);
@@ -213,7 +212,8 @@ class ScaledReordered
     std::shared_ptr<const LinOp> inner_operator_{};
     std::shared_ptr<const matrix::Diagonal<value_type>> row_scaling_{};
     std::shared_ptr<const matrix::Diagonal<value_type>> col_scaling_{};
-    std::shared_ptr<matrix::Permutation<index_type>> permutation_{};
+    std::shared_ptr<const array<index_type>> permutation_array_{};
+
     /**
      * Manages three vectors as a cache, so there is no need to allocate them
      * every time an intermediate vector is required. Copying an instance
@@ -226,19 +226,28 @@ class ScaledReordered
      */
     mutable struct cache_struct {
         cache_struct() = default;
+
         ~cache_struct() = default;
+
         cache_struct(const cache_struct&) {}
+
         cache_struct(cache_struct&&) {}
+
         cache_struct& operator=(const cache_struct&) { return *this; }
+
         cache_struct& operator=(cache_struct&&) { return *this; }
+
         std::unique_ptr<matrix::Dense<value_type>> inner_b{};
+
         std::unique_ptr<matrix::Dense<value_type>> inner_x{};
+
         std::unique_ptr<matrix::Dense<value_type>> intermediate{};
+
     } cache_;
 };
 
 
 }  // namespace gko
 
 
-#endif  // GKO_PUBLIC_CORE_BASE_REORDERED_HPP_
+#endif  // GKO_PUBLIC_CORE_BASE_SCALED_REORDERED_HPP_

include/ginkgo/ginkgo.hpp

@@ -62,7 +62,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <ginkgo/core/base/precision_dispatch.hpp>
 #include <ginkgo/core/base/range.hpp>
 #include <ginkgo/core/base/range_accessors.hpp>
-#include <ginkgo/core/base/reordered.hpp>
 #include <ginkgo/core/base/std_extensions.hpp>
 #include <ginkgo/core/base/temporary_clone.hpp>
 #include <ginkgo/core/base/temporary_conversion.hpp>
@@ -112,6 +111,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include <ginkgo/core/reorder/rcm.hpp>
 #include <ginkgo/core/reorder/reordering_base.hpp>
+#include <ginkgo/core/reorder/scaled_reordered.hpp>
 
 #include <ginkgo/core/solver/bicg.hpp>
 #include <ginkgo/core/solver/bicgstab.hpp>

reference/test/base/CMakeLists.txt

@@ -3,5 +3,4 @@ ginkgo_create_test(combination)
 ginkgo_create_test(composition)
 ginkgo_create_test(index_set)
 ginkgo_create_test(perturbation)
-ginkgo_create_test(reordered)
 ginkgo_create_test(utils)

reference/test/reorder/CMakeLists.txt

@@ -1,2 +1,3 @@
 ginkgo_create_test(rcm)
 ginkgo_create_test(rcm_kernels)
+ginkgo_create_test(scaled_reordered)