- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5
This fixes cmake for permutation in HyKKT #209
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
Conversation
| Added some fixes for CUDA | 
a39084e    to
    718e2e4      
    Compare
  
    718e2e4    to
    b6b1617      
    Compare
  
    | Successfully rebased with respect to  | 
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.
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 <[email protected]>
| @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. | 
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 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.