Simpler stopping criterion interface #1947
Conversation
ginkgo-bot
added
reg:testing
|
Note: ensure that the naming is the same as for the config parameters, see #1613 |
upsj
force-pushed
the
simpler_stop_interface
branch
from
e45cab5 to
b6bfe6e
Compare
|
|
||
| #include "ginkgo/core/stop/iteration.hpp" | ||
|
|
||
| #include "ginkgo/core/base/abstract_factory.hpp" |
There was a problem hiding this comment.
a few of the headers need fixing
- rename functions to match configuration keys - add documentation - add missing tests Co-authored-by: Marcel Koch <[email protected]> Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>
upsj
force-pushed
the
simpler_stop_interface
branch
from
f33c0d0 to
89fe7e0
Compare
MarcelKoch
left a comment
MarcelKoch
left a comment
There was a problem hiding this comment.
Only the testing of the correct dynamic type of the resulting residual stopping criterion is required. The rest are suggestions.
| }; | ||
|
|
||
|
|
||
| TEST_F(Iteration, NewInterface) |
There was a problem hiding this comment.
nit: this test could probably have a more descriptive name, e.g. SimplifiedInterface as the other ones, or FreeFunctionInterface.
| auto crit_red = | ||
| gko::as<gko::stop::ResidualNorm<TypeParam>>(factory_red->generate( | ||
| nullptr, rhs, initial_guess.get(), initial_res.get())); | ||
|
|
| auto crit_abs = | ||
| gko::as<gko::stop::ResidualNorm<TypeParam>>(factory_abs->generate( | ||
| nullptr, rhs, initial_guess.get(), initial_res.get())); | ||
| auto crit_rel = | ||
| gko::as<gko::stop::ResidualNorm<TypeParam>>(factory_rel->generate( | ||
| nullptr, rhs, initial_guess.get(), initial_res.get())); | ||
| auto crit_red = | ||
| gko::as<gko::stop::ResidualNorm<TypeParam>>(factory_red->generate( | ||
| nullptr, rhs, initial_guess.get(), initial_res.get())); |
There was a problem hiding this comment.
This should probably also be part of the Assert block. Especially since the value type is determined by the input arguments.
| } | ||
|
|
||
|
|
||
| TYPED_TEST(ImplicitResidualNorm, SimplifiedInterface) |
| } | ||
|
|
||
|
|
||
| TEST_F(Time, CanCreateFactoryNewInterface) |
| struct residual_norm_factory_parameters | ||
| : public enable_parameters_type<residual_norm_factory_parameters, | ||
| ResidualNormFactory> { | ||
| double GKO_FACTORY_PARAMETER_SCALAR(threshold, 0.0); |
There was a problem hiding this comment.
nit: maybe call this reduction_factor to be consistent with the original residual norm parameters.
| } | ||
| result = ResidualNorm<value_type>::build() | ||
| .with_baseline(this->parameters_.baseline) | ||
| .with_reduction_factor(this->parameters_.threshold) |
There was a problem hiding this comment.
nit:
| .with_reduction_factor(this->parameters_.threshold) | |
| .with_reduction_factor(cast_threshold) |
| }; | ||
|
|
||
|
|
||
| class ImplicitResidualNormFactory |
There was a problem hiding this comment.
If you want to reduce code duplication between this and the ResidualNormFactory, you could add a template for the actual stopping criteria type.
This replaces typed stopping criteria like
by the simpler
.with_criteria(gko::stop::abs_residual_norm(1e-5))