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

P1673: Overloading of symmetric_matrix_rank_k_update and similar functions is ambiguous #134

Closed
mhoemmen opened this issue Dec 8, 2021 · 3 comments
Labels
P1673 Issues relating to P1673 (esp. matching the proposal)

Comments

@mhoemmen
Copy link
Contributor

mhoemmen commented Dec 8, 2021

@youyu3 observed that functions like symmetric_matrix_rank_k_update take all possible combinations of (ExecutionPolicy&& or not, alpha or not). Without further constraints on ExecutionPolicy, this makes calls to the function ambiguous.

This is an issue in P1673 itself; we'll need some way to resolve this. The most straightforward way would be to require alpha unconditionally (even if it's always 1.0), and only overload on ExecutionPolicy&&.

@mhoemmen mhoemmen added the P1673 Issues relating to P1673 (esp. matching the proposal) label Dec 8, 2021
mhoemmen pushed a commit to mhoemmen/cpp-proposals-pub that referenced this issue Dec 15, 2021
Remove all overloads of `symmetric_matrix_rank_k_update` and
`hermitian_matrix_rank_k_update` that do not take an `alpha` parameter.
This prevents ambiguity between overloads
that take `ExecutionPolicy&&` but not `alpha`,
and overloads that take `alpha` but not `ExecutionPolicy&&`.

This fixes kokkos/stdBLAS#134 .
@mhoemmen
Copy link
Contributor Author

The issue with the proposal should be fixed in R6 (PR ORNL/cpp-proposals-pub#186 ). I'll file a separate issue for the implementation.

@mhoemmen
Copy link
Contributor Author

R6 was submitted with the overloads removed. However, per discussion with @crtrott , I'm leaving this issue open. We may want to bring back the overloads, if we can figure out how to constrain them (e.g., by saying that alpha has to work in a suitable arithmetic expression).

@mhoemmen
Copy link
Contributor Author

My PR #263 (merged today) fixes this in the implementation, and adds tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1673 Issues relating to P1673 (esp. matching the proposal)
Projects
None yet
Development

No branches or pull requests

1 participant