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

Preliminary command-line options parser #140

Merged
merged 7 commits into from
Feb 28, 2024
Merged

Preliminary command-line options parser #140

merged 7 commits into from
Feb 28, 2024

Conversation

pelesh
Copy link
Collaborator

@pelesh pelesh commented Feb 21, 2024

Provides:

  • Basic parser for command line options.
  • Single test for iterative solvers through SystemSolver interface

@pelesh pelesh added enhancement New feature or request testing labels Feb 21, 2024
@pelesh pelesh requested a review from kswirydo February 21, 2024 01:20
@pelesh pelesh self-assigned this Feb 21, 2024
@pelesh pelesh marked this pull request as draft February 21, 2024 01:20
@pelesh pelesh marked this pull request as ready for review February 22, 2024 00:36
@pelesh
Copy link
Collaborator Author

pelesh commented Feb 22, 2024

@kswirydo, after rebasing to the latest develop, unit tests for CGS1 pass, but functionality tests using CGS1 still fail, bot on CPU and GPU.

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.

Largely nitpicks - this is in great shape

resolve/SystemSolver.cpp Show resolved Hide resolved
resolve/SystemSolver.cpp Show resolved Hide resolved
resolve/SystemSolver.cpp Show resolved Hide resolved
resolve/CMakeLists.txt Outdated Show resolved Hide resolved
resolve/LinSolverIterativeRandFGMRES.cpp Show resolved Hide resolved
resolve/SystemSolver.cpp Show resolved Hide resolved
resolve/utilities/params/CMakeLists.txt Show resolved Hide resolved
resolve/utilities/params/CliOptions.cpp Show resolved Hide resolved
tests/functionality/CMakeLists.txt Outdated Show resolved Hide resolved
tests/functionality/testSysRandGMRES.cpp Show resolved Hide resolved
@pelesh pelesh requested a review from maksud February 26, 2024 19:42
Copy link
Collaborator

@kswirydo kswirydo left a comment

Choose a reason for hiding this comment

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

Looks good. Now I am running it to see how the CLO interface works in practice.

tests/functionality/testSysRandGMRES.cpp Show resolved Hide resolved
tests/functionality/testSysRandGMRES.cpp Show resolved Hide resolved
@kswirydo
Copy link
Collaborator

Okay, so I decided to try to break it:

./tests/functionality/sys_rand_gmres_test.exe -i randfgmres -s fwht -g cgs2
Segmentation fault (core dumped)

(should throw error if solver is wrong)
Also

./tests/functionality/sys_rand_gmres_test.exe -i fgmres -s fwht -g cgs2
Iterative solver results: 
	 Sketching method:                                    : fwht
	 Initial residual norm:          ||b-Ax_0||_2         : 7.8492037812761619e+03 
	 Initial relative residual norm: ||b-Ax_0||_2/||b||_2 : 1.0000000000000000e+00 
	 Final residual norm:            ||b-Ax||_2           : 7.2975483225447763e-09 
	 Final relative residual norm:   ||b-Ax||_2/||b||_2   : 9.2971829065677610e-13 
	 Number of iterations                                 : 19
Test PASSED

There should be NO sketching method is solver is FGMRES.

@kswirydo
Copy link
Collaborator

Also this (note there is no sketching method so it is set to default - but then the name is not displayed)

./tests/functionality/sys_rand_gmres_test.exe -i randgmres  -g cgs3
Sketching method  not recognized.
Setting sketching to the default (count).

[WARNING] Gram-Schmidt variant cgs2 not recognized.
[WARNING] Using default cgs2 Gram-Schmidt variant.
[WARNING] Sketching method  not recognized!
Using default.
Iterative solver results: 
	 Sketching method:                                    : 
	 Initial residual norm:          ||b-Ax_0||_2         : 7.8910952345032565e+03 
	 Initial relative residual norm: ||b-Ax_0||_2/||b||_2 : 1.0053370321875226e+00 
	 Final residual norm:            ||b-Ax||_2           : 1.1069282838889689e-08 
	 Final relative residual norm:   ||b-Ax||_2/||b||_2   : 1.4102427644055879e-12 
	 Number of iterations                                 : 19
Test PASSED

@pelesh
Copy link
Collaborator Author

pelesh commented Feb 28, 2024

If the option parameter is unrecognized, the test should fall back to the default. It should not segfault. I agree, sketching should not be printed, if randomized method is not used. This is easy to fix though.

@pelesh
Copy link
Collaborator Author

pelesh commented Feb 28, 2024

Also this (note there is no sketching method so it is set to default - but then the name is not displayed)

./tests/functionality/sys_rand_gmres_test.exe -i randgmres  -g cgs3
Sketching method  not recognized.
Setting sketching to the default (count).

[WARNING] Gram-Schmidt variant cgs2 not recognized.
[WARNING] Using default cgs2 Gram-Schmidt variant.
[WARNING] Sketching method  not recognized!
Using default.
Iterative solver results: 
	 Sketching method:                                    : 
	 Initial residual norm:          ||b-Ax_0||_2         : 7.8910952345032565e+03 
	 Initial relative residual norm: ||b-Ax_0||_2/||b||_2 : 1.0053370321875226e+00 
	 Final residual norm:            ||b-Ax||_2           : 1.1069282838889689e-08 
	 Final relative residual norm:   ||b-Ax||_2/||b||_2   : 1.4102427644055879e-12 
	 Number of iterations                                 : 19
Test PASSED

I cannot reproduce this. I get this when I make the same call:

$ ./tests/functionality/sys_rand_gmres_test.exe -i randgmres  -g cgs3
Unknown orthogonalization cgs3
Setting orthogonalization to the default (CGS2).

Results for randomized FGMRES solver
	 Sketching method:               count sketching
	 Orthogonalization method:       reorthogonalized classical Gram-Schmidt
	 Hardware backend:               HIP
	 Solver tolerance:               1e-12
	 Initial residual norm:          ||b-Ax_0||_2         : 7.9193852034106785e+03 
	 Initial relative residual norm: ||b-Ax_0||_2/||b||_2 : 1.0089412154519328e+00 
	 Final residual norm:            ||b-Ax||_2           : 5.0631784499176610e-09 
	 Final relative residual norm:   ||b-Ax||_2/||b||_2   : 6.4505631284482523e-13 
	 Number of iterations                                 : 19
Test PASSED

I pulled from the lates cli-params-dev.

@pelesh
Copy link
Collaborator Author

pelesh commented Feb 28, 2024

@kswirydo, I couldn't reproduce issues you reported. I will merge this but feel free to open an issue, if you still see these problems.

@pelesh pelesh merged commit f80a3fe into develop Feb 28, 2024
3 checks passed
@pelesh pelesh deleted the cli-params-dev branch March 6, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants