Skip to content

Commit

Permalink
remove virtual call in minres constructor + add test with preconditioner
Browse files Browse the repository at this point in the history
clang tidy warns about using virtual calls in constructors, an explaination for that can be found here: https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP50-CPP.+Do+not+invoke+virtual+functions+from+constructors+or+destructors
  • Loading branch information
MarcelKoch committed May 12, 2022
1 parent 907ebe8 commit 16fc9af
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 4 deletions.
10 changes: 6 additions & 4 deletions include/ginkgo/core/solver/minres.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,15 @@ class Minres : public EnableLinOp<Minres<ValueType>>,
if (parameters_.generated_preconditioner) {
GKO_ASSERT_EQUAL_DIMENSIONS(parameters_.generated_preconditioner,
this);
set_preconditioner(parameters_.generated_preconditioner);
Preconditionable::set_preconditioner(
parameters_.generated_preconditioner);
} else if (parameters_.preconditioner) {
set_preconditioner(
Preconditionable::set_preconditioner(
parameters_.preconditioner->generate(system_matrix_));
} else {
set_preconditioner(matrix::Identity<ValueType>::create(
this->get_executor(), this->get_size()));
Preconditionable::set_preconditioner(
matrix::Identity<ValueType>::create(this->get_executor(),
this->get_size()));
}
stop_criterion_factory_ =
stop::combine(std::move(parameters_.criteria));
Expand Down
34 changes: 34 additions & 0 deletions reference/test/solver/minres_kernels.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <ginkgo/core/base/exception.hpp>
#include <ginkgo/core/base/executor.hpp>
#include <ginkgo/core/matrix/dense.hpp>
#include <ginkgo/core/preconditioner/jacobi.hpp>
#include <ginkgo/core/stop/combined.hpp>
#include <ginkgo/core/stop/iteration.hpp>
#include <ginkgo/core/stop/residual_norm.hpp>
Expand Down Expand Up @@ -94,6 +95,19 @@ class Minres : public ::testing::Test {
gko::stop::ResidualNorm<value_type>::build()
.with_reduction_factor(r<value_type>::value / 2)
.on(exec))
.on(exec)),
preconditioned_minres_factory(
Solver::build()
.with_criteria(
gko::stop::Iteration::build().with_max_iters(40u).on(
exec),
gko::stop::ResidualNorm<value_type>::build()
.with_reduction_factor(r<value_type>::value / 2)
.on(exec))
.with_preconditioner(
gko::preconditioner::Jacobi<value_type>::build()
.with_max_block_size(1u)
.on(exec))
.on(exec))
{
stopped.stop(1);
Expand Down Expand Up @@ -135,6 +149,7 @@ class Minres : public ::testing::Test {
gko::Array<gko::stopping_status> small_stop;

std::unique_ptr<typename Solver::Factory> minres_factory;
std::unique_ptr<typename Solver::Factory> preconditioned_minres_factory;
};

TYPED_TEST_SUITE(Minres, gko::test::ValueTypes, TypenameNameGenerator);
Expand Down Expand Up @@ -308,4 +323,23 @@ TYPED_TEST(Minres, SolvesSystem)
}


TYPED_TEST(Minres, SolvesPreconditionedSystem)
{
using Mtx = typename TestFixture::Mtx;
using vt = typename TestFixture::value_type;
auto one_op = gko::initialize<Mtx>({gko::one<vt>()}, this->exec);
auto neg_one_op = gko::initialize<Mtx>({-gko::one<vt>()}, this->exec);
auto solver = this->preconditioned_minres_factory->generate(this->mtx);
auto x = gko::initialize<Mtx>({-1., 2., 3., 4.}, this->exec);
auto sol = gko::clone(this->exec, x);
auto b = Mtx::create(this->exec, x->get_size());
this->mtx->apply(x.get(), b.get());
x->fill(0.);

solver->apply(b.get(), x.get());

GKO_ASSERT_MTX_NEAR(x, sol, r<vt>::value * 10);
}


} // namespace
45 changes: 45 additions & 0 deletions test/solver/minres_kernels.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <ginkgo/core/base/exception.hpp>
#include <ginkgo/core/base/executor.hpp>
#include <ginkgo/core/matrix/dense.hpp>
#include <ginkgo/core/preconditioner/jacobi.hpp>
#include <ginkgo/core/stop/combined.hpp>
#include <ginkgo/core/stop/iteration.hpp>
#include <ginkgo/core/stop/residual_norm.hpp>
Expand Down Expand Up @@ -328,4 +329,48 @@ TEST_F(Minres, ApplyIsEquivalentToRef)
}


TEST_F(Minres, PreconditionedApplyIsEquivalentToRef)
{

auto mtx = gen_mtx(50, 50, 53);
gko::test::make_hpd(mtx.get());
auto x = gen_mtx(50, 1, 5);
auto b = gen_mtx(50, 1, 4);
auto d_mtx = gko::clone(exec, mtx);
auto d_x = gko::clone(exec, x);
auto d_b = gko::clone(exec, b);
auto minres_factory =
gko::solver::Minres<value_type>::build()
.with_criteria(
gko::stop::Iteration::build().with_max_iters(400u).on(ref),
gko::stop::ResidualNorm<value_type>::build()
.with_reduction_factor(::r<value_type>::value)
.on(ref))
.with_preconditioner(
gko::preconditioner::Jacobi<value_type>::build()
.with_max_block_size(1u)
.on(ref))
.on(ref);
auto d_minres_factory =
gko::solver::Minres<value_type>::build()
.with_criteria(
gko::stop::Iteration::build().with_max_iters(400u).on(exec),
gko::stop::ResidualNorm<value_type>::build()
.with_reduction_factor(::r<value_type>::value)
.on(exec))
.with_preconditioner(
gko::preconditioner::Jacobi<value_type>::build()
.with_max_block_size(1u)
.on(exec))
.on(exec);
auto solver = minres_factory->generate(std::move(mtx));
auto d_solver = d_minres_factory->generate(std::move(d_mtx));

solver->apply(b.get(), x.get());
d_solver->apply(d_b.get(), d_x.get());

GKO_ASSERT_MTX_NEAR(d_x, x, ::r<value_type>::value * 100);
}


} // namespace

0 comments on commit 16fc9af

Please sign in to comment.