Parameter management utilities #163

pelesh · 2024-06-13T16:43:10Z

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:

Create a generic parameter management interface at the base class, which each derived solver class can populate with its own specific parameters.
Parameters can be of different types (integer, floating point, string, etc.).
This is core functionality, no additional dependency should be required to implement it.
Changing parameters should leave solver in fully functional state with new parameter values.

This draft PR is a proposal how to handle this. Parameter management is implemented only for classes LinSolverIterativeFGMRES and LinSolverIterativeRandGMRES for now.

Closes #96

cameronrutherford

I don't really have substantial feedback...

resolve/LinSolver.hpp

cameronrutherford · 2024-06-14T20:19:32Z

tests/functionality/testSysRandGMRES.cpp

@@ -129,17 +129,23 @@ int test(int argc, char *argv[])
  real_type norm_b = 0.0;

  // Set solver options
-  real_type tol = 1e-12;
+  //real_type tol = 1e-12;


Suggested change

//real_type tol = 1e-12;

Good catch!

cameronrutherford · 2024-06-14T20:19:41Z

tests/functionality/testSysRandGMRES.cpp

  solver.getIterativeSolver().setMaxit(2500);
-  solver.getIterativeSolver().setTol(tol);
+  // solver.getIterativeSolver().setTol(tol);


Suggested change

// solver.getIterativeSolver().setTol(tol);

cameronrutherford · 2024-06-14T20:20:05Z

tests/functionality/testSysRandGMRES.cpp


  matrix_handler.setValuesChanged(true, memspace);

  // Set system matrix and initialize iterative solver
  status = solver.setMatrix(A);
  error_sum += status;

-  solver.getIterativeSolver().setRestart(200);
+  // solver.getIterativeSolver().setRestart(200);


Suggested change

// solver.getIterativeSolver().setRestart(200);

cameronrutherford · 2024-06-14T20:27:31Z

resolve/LinSolverIterativeRandFGMRES.cpp

+  int LinSolverIterativeRandFGMRES::getCliParam(const std::string id, std::string& /* value */)
+  {
+    switch (params_list_[id])
+    {
+      default:
+        out::error() << "Unknown string parameter " << id << "\n";
+        return 1;
+    }
+    return 0;
+  }


I am a little confused about the purpose of this function. Is that to say no parameters have a string as their value?

You are right. Randomized FGMRES solver does not have configuration parameters that are of string type. This method is just a placeholder. Need to document this better.

resolve/LinSolverIterativeFGMRES.cpp

stonecoldhughes · 2024-09-20T02:15:12Z

#96 (comment)

has this utility been tested for all input conditions? Is there logic to detect and report ill-formed combinations of options?

Config options are often represented naturally with a hierarchical data format, and this allows them to be saved for later use. Does that capability exist with this parser? If not, is it desirable?

https://github.com/nlohmann/json is a single header file, so it could be added to the repo once and never bothered with again.

https://github.com/nlohmann/json can parse JSON from a file or the command line directly via stdin if file I/O is undesirable.

Thousands of users have helped refine json.hpp, it is extremely robust and extensively tested.

stonecoldhughes · 2024-09-20T02:24:24Z

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?

pelesh · 2024-09-27T23:11:00Z

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.

stonecoldhughes

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?

stonecoldhughes · 2024-10-08T16:42:41Z

resolve/LinSolver.hpp

+        return 1;
+      }
+
+      virtual int getCliParam(const std::string /* id */, index_type& /* value */)


Could these functions be pure virtual? This is possible if and only if they are always overridden in derived classes; otherwise a default implementation like this is required.

pelesh · 2024-10-08T18:01:08Z

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.

pelesh added enhancement New feature or request question Further information is requested labels Jun 13, 2024

pelesh requested review from kswirydo, superwhiskers and cameronrutherford June 13, 2024 16:43

pelesh self-assigned this Jun 13, 2024

cameronrutherford reviewed Jun 14, 2024

View reviewed changes

pelesh requested review from stonecoldhughes and removed request for superwhiskers September 19, 2024 02:01

pelesh force-pushed the params-dev branch from 6d1b52e to e3268a8 Compare September 27, 2024 23:34

stonecoldhughes approved these changes Oct 8, 2024

View reviewed changes

cameronrutherford marked this pull request as ready for review October 8, 2024 17:40

pelesh marked this pull request as draft October 8, 2024 17:58

pelesh added 5 commits October 12, 2024 17:12

Separate files for linear solvers base classes.

4315a52

Set/get CLI parameters (first stab).

05a3227

Fixed parameter management bugs in RandFGMRES and FGMRES classes.

b5e1363

Add correct include file to LUSOL.

3206f99

Add unit test for setting/getting solver params.

6ac0bbd

pelesh force-pushed the params-dev branch from 5356946 to 6ac0bbd Compare October 12, 2024 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter management utilities #163

Parameter management utilities #163

pelesh commented Jun 13, 2024 •

edited

Loading

cameronrutherford left a comment

cameronrutherford Jun 14, 2024

pelesh Jun 14, 2024

cameronrutherford Jun 14, 2024

cameronrutherford Jun 14, 2024

cameronrutherford Jun 14, 2024

pelesh Jun 14, 2024 •

edited

Loading

stonecoldhughes commented Sep 20, 2024 •

edited

Loading

stonecoldhughes commented Sep 20, 2024

pelesh commented Sep 27, 2024

stonecoldhughes left a comment

stonecoldhughes Oct 8, 2024

pelesh commented Oct 8, 2024

Parameter management utilities #163

Are you sure you want to change the base?

Parameter management utilities #163

Conversation

pelesh commented Jun 13, 2024 • edited Loading

cameronrutherford left a comment

Choose a reason for hiding this comment

cameronrutherford Jun 14, 2024

Choose a reason for hiding this comment

pelesh Jun 14, 2024

Choose a reason for hiding this comment

cameronrutherford Jun 14, 2024

Choose a reason for hiding this comment

cameronrutherford Jun 14, 2024

Choose a reason for hiding this comment

cameronrutherford Jun 14, 2024

Choose a reason for hiding this comment

pelesh Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

stonecoldhughes commented Sep 20, 2024 • edited Loading

stonecoldhughes commented Sep 20, 2024

pelesh commented Sep 27, 2024

stonecoldhughes left a comment

Choose a reason for hiding this comment

stonecoldhughes Oct 8, 2024

Choose a reason for hiding this comment

pelesh commented Oct 8, 2024

pelesh commented Jun 13, 2024 •

edited

Loading

pelesh Jun 14, 2024 •

edited

Loading

stonecoldhughes commented Sep 20, 2024 •

edited

Loading