-
Notifications
You must be signed in to change notification settings - Fork 101
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
Staggered invert test cleanup + expanded staggered gtest support #1421
Conversation
… links/other missing things
…stent with Wilson-type verifies
…merated test types, set new defaults, improved command line arg documentation
…gered_dslash_test_utils
…e test and ctest flavors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments based on visual code review.
Beyond that though, my main concern is how long the tests take to run, noting that the tests are failing to complete on CSCS. In particular:
- Staggered eigensolves take much longer than their Wilson brethren. Anything to be done here?
- HISQ stencil test looks like it's taking longer than expected: is there unnecessary setup being redone between partitions?
Based on offline conversations, the time spent on staggered eigensolves has been addressed in a1303bd , plus some additional opportunities to speed up the unit tests run on CSCS in 3252b6c . The HISQ stencil test time was decided to be reasonable. |
…e running eigensolver dslash tests on the improved staggered operator, which are expensive
Got a few things to fix in BiCGstab with regards to MG (near-null vector generation and pre-smoothing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this PR, subject to the HISQ stencil tests being merged into a single test (as opposed to separate test/ctest driver files #1430). Great work @weinbe2.
@cpviolator before we merge this, want to take a quick look at the Eigensolver changes made in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice adjustment @weinbe2 ! Perfectly happy to see convergence and tolerance adherence improving.
This PR is a massive cleanup and refactor of various staggered test executables:
staggered_dslash_(c)test
,staggered_invert_test
,staggered_eigensolve_test
, andhisq_stencil_test
. Motivators:gtest
support in all of themdslash_(c)test
,invert_test
, andeigensolve_test
) as possible-DQUDA_LAPLACE
)Some cleanup happened along the way:
ColorSpinorField
objects as opposed to pointers thereofstaggered_invert_test
ctests uncovered this!is_[...]_[solve/solution/etc]
routines out of the invert test helpers and into the more generalhost_utils
files.A few gotchas of note:
staggered_invert_test
is now to solve the preconditioned operator with conjugate gradient, since that's arguably the most typical use case. The previous default was solving the full operator with bicgstab.--test [#]
flags fromstaggered_invert_test
to more closely match the behavior ofinvert_test
. I created a new flag--legacy-test-info
that will print the appropriate command line arguments to reproduce each old test.I did make a few changes in preparation of adding support for the "local" staggered operators, which can be used for Schwarz preconditioning. This includes copying and pasting some relevant bits of code from Wilson-type files into the staggered-type files, commenting the code out, and explicitly leaving an additional comment that this is for future reference. I know this is generally not a best practice, but hopefully it's understandable in this case.
As it stands, a staggered-only build does indeed pass
ctest -v
.There are some small bits of outstanding work, but nothing I see as holding up PR review right now. They are:
ctest -v
for Wilson-type fermions still works (this PR nearly exclusively effects staggered tests, but I did introduce a few abstractions that maybe went awry for Wilson)staggered_invert_test
staggered_invert_test
doxygen
clang-format