Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameter management utilities #163

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Parameter management utilities #163

wants to merge 10 commits into from

Conversation

pelesh
Copy link
Collaborator

@pelesh pelesh commented Jun 13, 2024

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 PR is a proposal how to handle this. Parameter management is implemented only for classes LinSolverIterativeFGMRES and LinSolverIterativeRandGMRES. After this is agreed upon and merged, parameter management will be added to other solvers.

@pelesh pelesh added enhancement New feature or request question Further information is requested labels Jun 13, 2024
@pelesh pelesh self-assigned this Jun 13, 2024
Copy link
Collaborator

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have substantial feedback...

resolve/LinSolver.hpp Show resolved Hide resolved
tests/functionality/testSysRandGMRES.cpp Outdated Show resolved Hide resolved
tests/functionality/testSysRandGMRES.cpp Outdated Show resolved Hide resolved
tests/functionality/testSysRandGMRES.cpp Outdated Show resolved Hide resolved
resolve/LinSolverIterativeRandFGMRES.cpp Outdated Show resolved Hide resolved
resolve/LinSolverIterativeFGMRES.cpp Outdated Show resolved Hide resolved
@pelesh pelesh requested review from stonecoldhughes and removed request for superwhiskers September 19, 2024 02:01
@stonecoldhughes
Copy link
Collaborator

stonecoldhughes commented Sep 20, 2024

#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
Copy link
Collaborator

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
Copy link
Collaborator Author

pelesh commented Sep 27, 2024

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.

Copy link
Collaborator

@stonecoldhughes stonecoldhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

resolve/LinSolver.hpp Show resolved Hide resolved
@cameronrutherford cameronrutherford marked this pull request as ready for review October 8, 2024 17:40
@pelesh pelesh marked this pull request as draft October 8, 2024 17:58
@pelesh
Copy link
Collaborator Author

pelesh commented Oct 8, 2024

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
Copy link
Collaborator Author

pelesh commented Dec 17, 2024

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?

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.

@pelesh
Copy link
Collaborator Author

pelesh commented Dec 17, 2024

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?

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.

@pelesh pelesh marked this pull request as ready for review December 17, 2024 03:58
@pelesh pelesh requested a review from shakedregev December 17, 2024 04:10
Copy link
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #203, for example. We do need to make these tests more modular and more configurable.

@pelesh pelesh requested review from maksud and nkoukpaizan December 20, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants