This fixes cmake for permutation in HyKKT#209
Conversation
|
Added some fixes for CUDA |
a39084e to
718e2e4
Compare
718e2e4 to
b6b1617
Compare
|
Successfully rebased with respect to |
pelesh
left a comment
There was a problem hiding this comment.
This is a very nice first step for porting HyKKT to Re::Solve. I am converting this to a draft PR as it may need a bit more work to get ready to merge. Some immediate actions that I would suggest:
- Use
ReSolvename space for all HyKKT classes. - Consider keeping HyKKT specific code in
hykktnamespace withinReSolve. - Make sure CI tests pass. Currently there seem to be some issues due to incorrect include paths.
|
* Coding style suggestions. * added param in/out * Style suggestions for permutation tests. * Some more doxygen formatting. --------- Co-authored-by: shakedregev <shakedvregev@gmail.com>
|
@pelesh - I merged your other branch into this after testing on Frontier and Nvidia machines. Please let me know if any more changes are needed here. |
There was a problem hiding this comment.
Looks good! Some minor corrections are needed before merging:
- Complete all Doxygen comments. Make sure there are no empty fields.
- Use
switchstatement instead of preprocessor directives to select sort method. - I believe file
runMatrixPermutationTests.cppcan be removed. It seems it was added accidentally.
|
@pelesh - I've addressed your comments, please let me know if you have any more or if we are ready to merge. |
pelesh
left a comment
There was a problem hiding this comment.
I added a few nitpicking comments that would be good to address, but I think we can merge this.
In this branch we port
perm_testfromHyKKTtoReSolveto test the first CPU based version of HyKKT tests. ReSolve formatting is adhered to. All tests work and build on the commit before last.Note that in order to resolve a merge conflict, I added another directory in a cmake file which breaks the code when using the last commit. That said, the new
hykkt-devbranch should work, because it includes theparamstest.