Parameter management utilities #163
Conversation
pelesh
added
enhancement
pelesh
requested review from
stonecoldhughes
and removed request for
superwhiskers
|
|
https://en.cppreference.com/w/cpp/language/raii is an idea that applies to configuration - ideally, an object is completely configured and ready to use as soon as it is constructed. Are these configurable solver objects configured with the parsed configuration options in their constructors? Or is the functionality here used to set that up after the fact? |
Since Re::Solve is used for solving series of similar linear systems it is conceivable that one might want to change its parameters at runtime. In that case, Re::Solve would need to ensure that the choice of solver configuration parameters is self-consistent and that correct workspace is allocated. |
There was a problem hiding this comment.
These changes are a big step in the right direction. According to https://en.wikipedia.org/wiki/Single-responsibility_principle, it is usually best to map each separate definable functionality to a separately testable object. The LinSolver base class appears to handle 3 things: parameter management, norm/residual calculation, and matrix equation solution. Is it possible for LinSolver to offload the first two into other classes to a greater degree? Would that increase the composability of the design in a desirable way?
|
I suggest we keep this as draft for now. We need to merge #198 and probably do some other fixes as we discover them through this preview PR. Thanks all for useful feedback so far. |
Norm/residual calculation is handled by matrix and vector handlers. Parameter management has two parts -- (1) conversion of user input to internal enums and (2) reallocating workspace and resetting solver for the new configuration. Part (1) could (and likely will) be separated out, but it is less than 10% of the overall work. In this PR the priority is to get (2) right. |
This is a very good point. In sparse numerical linear algebra, one needs to compute the size of the workspace after all the objects are instantiated. Similarly, changing some of the configuration parameters requires recomputing and reallocating the workspace. Therefore, solver object typically is not ready to use as soon as it is constructed. |
There was a problem hiding this comment.
I pulled and ran the new test, it works (old tests are also not broken). The code is understandable, but we should be able to find a way to make these MRs more manageable, as much of the code was repeated.
This seems built specifically for direct solves with iterative refinement. HyKKT has gamma, deltas, and the CG tolerance. Not sure if we want it to be general enough to include these types of solvers.
| @@ -177,7 +198,7 @@ int test(int argc, char *argv[]) | |||
| std::cout << "Result is not a finite number!\n"; | |||
| error_sum++; | |||
| } | |||
| if (final_norm/norm_b > (10.0 * tol)) { | |||
| if (final_norm/norm_b > (10.0 * tol_out)) { | |||
There was a problem hiding this comment.
Maybe this 10.0 should be an optional parameter? I can imagine a scenario where I specify a small tolerance, but don't necessarily want to throw an error if I wasn't able to achieve it (or 10X it)
There was a problem hiding this comment.
See #203, for example. We do need to make these tests more modular and more configurable.
Managing different solver parameters in Re::Solve becomes a nontrivial task and there is a need for consistent parameter management across all Re::Solve modules. In addition to setting up file I/O for initializing solver parameters, the scope here includes creating a common interface for setting and retrieving parameters for all solver classes and ability to change solver configuration parameters at runtime. The requirements are:
This PR is a proposal how to handle this. Parameter management is implemented only for classes
LinSolverIterativeFGMRESandLinSolverIterativeRandGMRES. After this is agreed upon and merged, parameter management will be added to other solvers.