Factory improvements

[upsj]

• Make all iterative solver parameter classes based on the same structure
• Allow passing factory parameter instances instead of full factories for criteria, preconditioners etc.
• Remove now unnecessary .on(...) calls.
• More strict const-correctness requirements for with_* functions

TODO:

• Test the old and new behavior
• Test with_logger propagating to factory and product
• Adapt Multigrid

Closes #766
Partially addresses #1162

[yhmtsai]

TBH, I prefer the old macro way to do this job.
the proposed way contains many parts which need to be checked manually.
for example,
std::shared_ptr<LinOpFactory> GKO_FACTORY_PARAMETER_SCALAR(factorization_factory, nullptr);
we get the type, scalar/vector, parameter name (including with_function), the default value in a place

std::shared_ptr<const LinOpFactory> factorization_factory{};
parameters_type& with_factorization(
            std::shared_ptr<const LinOpFactory> factory)
        {
            this->factorization_generator =
                [factory](std::shared_ptr<const Executor>)
                -> std::shared_ptr<const LinOpFactory> { return factory; };
            return *this;
        }
// another part for accepting without `.on(exec)`

the overload for supporting with/without factory can be handled in macro, too.
I will also suggest the deprecated factory parameter in another pr.

[upsj]

@yhmtsai This is really hard to do consistently, since there are many different semantics associated with the factory-type parameters:

• Required parameters in vectors (criterion, mg_level, ...)
• Optional parameters in vectors (smoother)
• Required plain parameters (inner_solver)
• Optional plain parameters (preconditioner)

All of those need to be handled differently in the .on(...) function call and potentially in the with_* function. There is only a small handful of types that need this implemented (Solver Base, Schwartz, Ilu/Ic preconditioner, Ir, Direct) that are sufficiently different that it's not possible to just use a macro for it.

I simplified things a bit using a deferred_factory_parameter struct which handles the std::function stuff, but it can still not be put into macros entirely.

[MarcelKoch]

I think this needs to be rebased to remove the changes from #1337

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
30 Code Smells

92.4% Coverage
11.4% Duplication

[yhmtsai]

I think there are two macro that can represent these changes.

type GKO_DEFERRED_FACTORY_PARAMETER(name, default) 
type name{default};
const parameters_type& with_##name(deferred_factory_parameter<LinOpFactory> _value) const {
  this->_name##_generator_ = std::move(_value);
}
private:
deferred_factory_parameter<const LinOpFactory> _name##_generator_;
public:

and corresponding vector version.

[yhmtsai]

I think the required and optional parameters should be checked by the LinOp self like current state.

[upsj]

What I'm talking about can best be seen in the Multigrid::factory_parameters::on function: Different parameters need to be handled differently on whether they support nullptr

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

78.0% Coverage
10.6% Duplication

The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[MarcelKoch]

Most of my suggestions are optional. The one I would like to see addressed is the deferred factory constructor with nullptr.

[MarcelKoch]

only some comments on documentation left

[pratikvn]

Mostly looks good to me. I am a bit concerned that the large number of changes might break interface, and we currently have no way of checking that as we update all the tests as well.

Maybe we should have something like interface stability tests, that ensure that the updated code does not break existing interface. Something similar to test_install would probably be sufficient and we dont need to make it elaborate.

[upsj]

format!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
21 Code Smells

71.3% Coverage
9.3% Duplication

The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[codecov]

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
core/distributed/preconditioner/schwarz.cpp	92.30% <100.00%> (ø)
core/test/log/convergence.cpp	96.15% <100.00%> (ø)
core/test/log/papi.cpp	97.42% <100.00%> (ø)
core/test/log/profiler_hook.cpp	95.80% <100.00%> (ø)
core/test/log/record.cpp	100.00% <100.00%> (ø)
core/test/log/stream.cpp	99.27% <100.00%> (ø)
...re/test/mpi/distributed/preconditioner/schwarz.cpp	100.00% <100.00%> (ø)
core/test/preconditioner/ic.cpp	100.00% <100.00%> (ø)
core/test/preconditioner/ilu.cpp	100.00% <100.00%> (ø)
core/test/solver/bicg.cpp	100.00% <100.00%> (ø)
... and 63 more

... and 7 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.