Fix symmetric_matrix_rank_k_update, hermitian_matrix_rank_k_update, and symmetric_matrix_rank_1_update #263
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Header and namespace seem to have changed since last time.
Constrain symmetric_matrix_rank_k_update ExecutionPolicy overloads so that the compiler can distinguish them from alpha overloads.
Algorithm was giving incorrect results. This commit fixes that and improves the associated regression test.
Add a regression test that didn't build before this change, and now builds correctly.
1. Fix ambiguous overload for the no-scalar case 2. Fix the algorithm (so it gives the right answer, for both the scalar and no-scalar cases)
1. Fix ambiguous overload for the no-scalar case 2. Fix the algorithm (so it gives the right answer for the no-scalar case; the with-scalar case was already fixed in a previous commit)
Add a new trait for testing whether an ExecutionPolicy is a valid execution policy (either std::is_execution_policy_v or custom). Use it in symmetric_matrix_rank_k_update and hermitian_matrix_rank_k_update.
The reference implementation needs to implement all constraints of symmetric_matrix_rank_1_update in order to disambiguate overloads. Add a regression test as well.
mhoemmen
changed the title
mhoemmen
changed the title
dalg24
reviewed
Jan 15, 2024
| @@ -4,8 +4,9 @@ | |||
| // This must be defined before including any mdspan headers. | |||
| #define MDSPAN_USE_PAREN_OPERATOR 1 | |||
|
|
|||
| #include <experimental/mdspan> | |||
| #include "experimental/__p2630_bits/submdspan.hpp" | |||
There was a problem hiding this comment.
Why explicitly include __p2630_bits/submdspan.hpp?
There was a problem hiding this comment.
@dalg24 It didn't build (with GCC 11.4 + CMake 3.22.1, otherwise default settings) otherwise. I really just did this:
cmake ../../../src/kokkos/stdBLAS -DLINALG_ENABLE_TESTS=ON -DLINALG_ENABLE_EXAMPLES=ON
make -j12
and got errors about not finding submdspan. Note that there's no <experimental/submdspan> header to include.
There was a problem hiding this comment.
I think the issue might be that we were meant to include <mdspan/mdspan.hpp>
There was a problem hiding this comment.
None of the following headers exist.
experimental/submdspanexperimental/submdspan.hppmdspan/submdspanmdspan/submdspan.hpp
There was a problem hiding this comment.
Shouldn't <mdspan/mdspan.hpp> include the submdspan headers?
| @@ -17,7 +18,8 @@ | |||
| using std::experimental::extents; | |||
| using std::full_extent; // not in experimental namespace | |||
| using std::experimental::mdspan; | |||
| using std::experimental::submdspan; | |||
|
|
|||
| using MDSPAN_IMPL_STANDARD_NAMESPACE :: submdspan; | |||
There was a problem hiding this comment.
Formatted weirdly. Also why is it not available from the same namespace as mdspan?
There was a problem hiding this comment.
Please see above comment (it really didn't build otherwise).
mhoemmen
changed the title
dalg24
approved these changes
Jan 16, 2024
This was referenced Jan 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this pull request fixes:
Add a trait that identifies an ExecutionPolicy as a valid (possibly custom) execution policy. (Note that it's forbidden for users to specialize
std::is_execution_policy_v.)Fix build errors when calling the following functions with or without an
alpha(but no execution policy), by constrainingExecutionPolicyoverloads (see Ambigious Template Definitions For "symmetric_matrix_rank_k_update" #261)symmetric_matrix_rank_k_updatehermitian_matrix_rank_k_updatesymmetric_matrix_rank_1_updateFix actual bugs in both the with-alpha and no-alpha
symmetric_matrix_rank_k_updateandhermitian_matrix_rank_k_updateoverloads that were giving the wrong answersDrive-by fix for
submdspan-related build errors in tests and examples, probably due to reference mdspan namespace changes that haven't been tested in this repository recentlyWhat this pull request does not fix or do:
ExecutionPolicyoverloads of all the algorithmsFixes #261.