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

Functionality tests utils dev #203

Draft
wants to merge 49 commits into
base: develop
Choose a base branch
from

Conversation

stonecoldhughes
Copy link
Collaborator

@stonecoldhughes stonecoldhughes commented Oct 16, 2024

Linked issue: #193

note: this PR has been opened and closed a few times due to insufficient testing before pushing. The others did not pass the checks. This PR applies a testing helper class to remove boilerplate from a single testing file

@stonecoldhughes stonecoldhughes self-assigned this Oct 16, 2024
Copy link
Collaborator

@superwhiskers superwhiskers left a comment

Choose a reason for hiding this comment

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

some minor things that stuck out to me that may be worth considering

tests/functionality/FunctionalityTestHelper.cpp Outdated Show resolved Hide resolved
namespace ReSolve {
namespace tests{

int FunctionalityTestHelper::checkRefactorizationResult(ReSolve::matrix::Csr& A,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name checkRefactorizationResult doesn't seem to be too appropriate here. while the only current exception is lusol, it may be more appropriate to name this checkResult, checkFactors, or something along those lines if this is to be more general in the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

checkResult seems simple and self-explanatory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed this to checkResultNorms() to be more specific

Copy link
Collaborator

@pelesh pelesh Oct 21, 2024

Choose a reason for hiding this comment

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

Checking error norms is how one would check the result, so checkResult will be as informative and less redundant, imho.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to checkResult

@stonecoldhughes stonecoldhughes force-pushed the functionality-tests-utils-dev branch from 6e485dc to a933508 Compare October 17, 2024 23:31
Comment on lines 84 to 85
// Verify relative residual norm computation in SystemSolver
error_sum += checkRelativeResidualNorm(vec_rhs, vec_x, residual_norm_, rhs_norm_, solver);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this call in testSysRefactor.cpp rather than here? Perhaps there will be an instance where we want to just check the result and not norm computation.

Copy link
Collaborator Author

@stonecoldhughes stonecoldhughes Oct 21, 2024

Choose a reason for hiding this comment

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

this function is now invoked in the main testing file

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would invoke it before checkResult. We want to establish first that SystemSolver computes error norms correctly before we test its solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines 68 to 71
// Compute norm of scaled residuals:
// NSR = ||r||_inf / (||A||_inf * ||x||_inf)
error_sum += checkNormOfScaledResiduals(A, vec_rhs, vec_x, vec_r, solver);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The call to checkNormOfScaledResiduals should probably be made from testSysRefactor.cpp instead of here. I would make this function check the result only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this function is now invoked in the main testing file

Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@stonecoldhughes
Copy link
Collaborator Author

stonecoldhughes commented Oct 29, 2024

note: the following improvements extend the work so far, but should probably be in a separate PR:

  • use same workspace in test_helper that is used in main test
  • create class to contain the matrix and vectors in the problem
  • re-order function calls in the test file to flow more logically
  • improve RAII in this file for code robustness; one ax=b problem and one testhelper exist for one test result. Use move constructors to preserve memory efficiency
  • configure test helper to calculate norms upon construction; as an explicit step, norm calculation is easy to forget and would make subsequent checks involving the norms ill-formed
  • use smart pointers in the test helper instead of raw pointers; this removes need for manual new/delete style memory management
  • break up the testing file into more functions with file-scope in the test to make main() more succinct. The top level naturally lends itself to a test_factorize() function followed by a test_refactorize()

@pelesh pelesh marked this pull request as draft December 16, 2024 22:14
@pelesh pelesh mentioned this pull request Dec 18, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants