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.

Sign up for GitHub

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

Jump to bottom

This fixes cmake for permutation in HyKKT #209

Merged
merged 30 commits into from
Jan 15, 2025
Merged

This fixes cmake for permutation in HyKKT #209

merged 30 commits into from
Jan 15, 2025

Conversation

shakedregev
Copy link
Collaborator

@shakedregev shakedregev commented Dec 27, 2024 •
edited
Loading

In this branch we port perm_test from HyKKT to ReSolve to 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-dev branch should work, because it includes the params test.

@shakedregev shakedregev requested a review from pelesh December 28, 2024 00:41
@shakedregev
Copy link
Collaborator Author

Added some fixes for CUDA

@pelesh pelesh closed this Dec 29, 2024
@pelesh pelesh reopened this Jan 3, 2025
@pelesh pelesh changed the base branch from develop to hykkt-dev January 3, 2025 19:28
@pelesh pelesh added the enhancement New feature or request label Jan 3, 2025
@pelesh pelesh marked this pull request as draft January 3, 2025 19:29
@shakedregev shakedregev marked this pull request as ready for review January 3, 2025 23:03
@shakedregev shakedregev force-pushed the slaven/permutation_cpu_port branch from a39084e to 718e2e4 Compare January 3, 2025 23:28
@shakedregev shakedregev force-pushed the slaven/permutation_cpu_port branch from 718e2e4 to b6b1617 Compare January 3, 2025 23:46
@shakedregev
Copy link
Collaborator Author

Successfully rebased with respect to hykkt-dev to resolve conflicts.

pelesh
pelesh reviewed Jan 7, 2025
View reviewed changes
resolve/CMakeLists.txt Outdated Show resolved Hide resolved
pelesh
pelesh requested changes Jan 7, 2025
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a 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 ReSolve name space for all HyKKT classes.
  • Consider keeping HyKKT specific code in hykkt namespace within ReSolve.
  • Make sure CI tests pass. Currently there seem to be some issues due to incorrect include paths.

@pelesh pelesh marked this pull request as draft January 7, 2025 19:54
@shakedregev
Copy link
Collaborator Author

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 ReSolve name space for all HyKKT classes.
  • Consider keeping HyKKT specific code in hykkt namespace within ReSolve.
  • Make sure CI tests pass. Currently there seem to be some issues due to incorrect include paths.
  1. I assume you mean more than just using it, right? Should I replace the vectors and matrices with the Resolve objects?
  2. ok, though this is usually the entire file
  3. Made some changes, let's see if it passes

@shakedregev shakedregev marked this pull request as ready for review January 7, 2025 22:41
@shakedregev shakedregev requested a review from pelesh January 7, 2025 22:45
@pelesh pelesh requested a review from maksud January 8, 2025 18:20
shakedregev and others added 5 commits January 8, 2025 20:28
@shakedregev
fixed naming
91c0f04
@shakedregev
function alignment
4eb17c2
@shakedregev
namespace fixes, const where possible
fa242c1
@pelesh
Fix issue with linking workspace to HyKKT. (#211)
7c013d3
@pelesh @shakedregev
Coding style suggestions (#213)
08d08b1 
* Coding style suggestions.

* added param in/out

* Style suggestions for permutation tests.

* Some more doxygen formatting.

---------

Co-authored-by: shakedregev <[email protected]>
@shakedregev
Copy link
Collaborator Author

@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.

pelesh
pelesh requested changes Jan 14, 2025
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a comment •
edited
Loading

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 switch statement instead of preprocessor directives to select sort method.
  • I believe file runMatrixPermutationTests.cpp can be removed. It seems it was added accidentally.

resolve/hykkt/Permutation.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.hpp Outdated Show resolved Hide resolved
tests/unit/hykkt/HykktPermutationTests.hpp Outdated Show resolved Hide resolved
tests/unit/hykkt/runHykktPermutationTests.cpp Outdated Show resolved Hide resolved
tests/unit/matrix/runMatrixPermutationTests.cpp Outdated Show resolved Hide resolved
@shakedregev
Copy link
Collaborator Author

@pelesh - I've addressed your comments, please let me know if you have any more or if we are ready to merge.

pelesh
pelesh approved these changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a 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.

resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
resolve/hykkt/cpuPermutationKernels.cpp Outdated Show resolved Hide resolved
@shakedregev shakedregev merged commit 393fa92 into hykkt-dev Jan 15, 2025
4 checks passed
@shakedregev shakedregev deleted the slaven/permutation_cpu_port branch January 15, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pelesh pelesh pelesh approved these changes

@maksud maksud Awaiting requested review from maksud

Assignees

@shakedregev shakedregev

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shakedregev @pelesh