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

symmetric_matrix_rank_1_update: kokkos impl #167

Conversation

mzuzek
Copy link

@mzuzek mzuzek commented Apr 8, 2022

This is Kokkos implementation of symmetric_matrix_rank_1_update.

@mzuzek mzuzek requested a review from fnrizzi April 8, 2022 11:27
@mzuzek mzuzek force-pushed the blas2_symmetric-matrix-rank1-update_kokkos_impl branch 2 times, most recently from d707a71 to 9903b48 Compare April 8, 2022 11:47
@mhoemmen
Copy link
Contributor

mhoemmen commented Apr 8, 2022

@fnrizzi has been touching a lot of this code of late, but it looks like you know that : - ) I'll wait until you two say it's ready to merge.

@fnrizzi
Copy link
Contributor

fnrizzi commented Apr 8, 2022

@mhoemmen yes, I have a plan with @MikolajZuzek so he is doing all the *_update functions :) so you can safely review this since it does not interfere with anything i am doing!

Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

The bug in is_int_mul_safe is a "must change"; other comments are suggestions. Thanks!

@mhoemmen
Copy link
Contributor

@MikolajZuzek Thanks for working on this, btw! : - )

@mzuzek mzuzek force-pushed the blas2_symmetric-matrix-rank1-update_kokkos_impl branch 2 times, most recently from 1c06d51 to c209631 Compare April 13, 2022 20:26
Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! I'm OK merging this, but there are a couple issues remaining:

  1. some use of std::complex in a KOKKOS_LAMBDA; not sure if OK; and
  2. a couple spelling errors in comments.

If you're in a hurry, I'll be happy to merge this, else I'll wait for fixes. Thanks!

@mzuzek
Copy link
Author

mzuzek commented Apr 13, 2022

If you're in a hurry, I'll be happy to merge this, else I'll wait for fixes. Thanks!

@mhoemmen Thank you for your thorough reviews and great comments! No hurry on my part: it's better to find and fix as many things as we can, so they don't get duplicated on other PRs.

@mzuzek mzuzek force-pushed the blas2_symmetric-matrix-rank1-update_kokkos_impl branch 3 times, most recently from bde2330 to 83d1742 Compare April 15, 2022 14:40
@mzuzek mzuzek force-pushed the blas2_symmetric-matrix-rank1-update_kokkos_impl branch from 83d1742 to 5287348 Compare April 15, 2022 14:46
@mzuzek mzuzek force-pushed the blas2_symmetric-matrix-rank1-update_kokkos_impl branch from 5287348 to 3628b87 Compare April 15, 2022 14:55
Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@mhoemmen mhoemmen merged commit b39e7c6 into kokkos:main Apr 15, 2022
@mzuzek mzuzek deleted the blas2_symmetric-matrix-rank1-update_kokkos_impl branch April 20, 2022 20:56
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