-
Notifications
You must be signed in to change notification settings - Fork 99
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
Fixes while documenting #2466
Fixes while documenting #2466
Conversation
That check is stricter than required as we will values by reference to perform copies and won't try to reassign pointers. Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
Signed-off-by: Luc Berger-Vergiat <[email protected]>
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.
@lucbv just to confirm, the rot, gerr and trmm routines are not intended to be compatible with DynRankView usage (based on inclusion of the static_assert(Kokkos::is_view_v...)
constraint)?
All of BLAS is pretty much asserting on is_view_v, the DynRankView use case was for Batched algorithms |
The cosine coefficient is strictly real while the sine coefficient can be real or complex leading to a bug in the current API. This commit should fix that for the native and TPL implementation and the associated unit-test is also fixed accordingly. Signed-off-by: Luc Berger-Vergiat <[email protected]>
The types for the arguments c and s are actually different and need to be appropriately propagated through the TPL layers of the library. Signed-off-by: Luc Berger-Vergiat <[email protected]>
9169bdc
to
f469bac
Compare
@brian-kelley can you have a look at this, there are changes for rot based on the discussion we had about the appropriate types required. |
@lucbv, I just got these errors:
I am not sure if the errors are caused by this merge. My cmake configurations: Kokkos: Kokkos Kernels: |
@vqd8a thanks for reporting it, I am having a look now! |
@lucbv Thanks. I think, changing |
I think the change that Vinh suggested makes sense though, the error also points at a cuDoubleComplex vs double issue which would be triggered by the change to a complex parameter for |
While writing the documentation I read a good amount of our public API and these are changes that we might want to consider to make things a bit more uniform across the library.