batched - dense: Testing and fixing Serial QR #2342

lucbv · 2024-09-19T19:45:54Z

The serial QR algorithms does not have unit-tests and is failing for non square matrices. See issue #2328.
This first commit fixes the issue with rectangular matrices and adds a basic test for that use case.
Next will work on adding a test that exercises the interfaces on multiple matrices of different sizes within a parallel_for. Finally equivalent tests will be added for the square case as well.

Tasks:

cwpearson

edit - duplicated comment

batched/dense/impl/KokkosBatched_QR_FormQ_Serial_Internal.hpp

batched/dense/unit_test/Test_Batched_SerialQR.hpp

The serial QR algorithms does not have unit-tests and is failing for non square matrices. See issue kokkos#2328. This first commit fixes the issue with rectangular matrices and adds a basic test for that use case. Next will work on adding a test that exercises the interfaces on multiple matrices of different sizes within a parallel_for. Finally equivalent tests will be added for the square case as well. Fixing unused variable error It looks like the Left NoTranspose ApplyQ is not doing the correct thing. Will have a look at that next. Spliting the tests a bit better, looking at Q and Qt together to detect issues with consistency between the two. Also eventually allows to use GEMM to figure out which one is wrong. Final fixes and adjusting the tolerance Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

yasahi-hpc

I requested small changes. I also have three additional points.

It seems that Side::Right specialization of KokkosBatched::SerialApplyQ is not tested. Is it possible to add a test case for this specialization?
Could you also consider to add docstrings in APIs under KokkosBatched_ApplyQ_Decl.hpp and KokkosBatched_QR_Decl.hpp? It would be helpful to generate API reference
Could you also add static_assertion to check the template argument is valid, e.g., check if ArgTrans is NoTranspose or Transpose.

batched/dense/impl/KokkosBatched_ApplyQ_Serial_Impl.hpp

batched/dense/unit_test/Test_Batched_SerialQR.hpp

Checking that the side, mode and algo are all valid. Cleaning up the test by using create_mirror_view_and_copy when appropriate. Somehow the semantic of create_mirror_view_and_copy is different from the regular deep_copy so need to make a few changes after testing on GPUs... Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

lucbv · 2025-03-10T23:23:24Z

I addressed 1. and 3. however for the documentation I will delay until we have it setup in the repo for batched algorithms.
This should not take too long as I am in the process of writing the main page for that and then we will be able to add the APIs one by one as we get to them.

yasahi-hpc

LGTM.
As for the documentation, I can contribute if needed.
Let me know if you decided how to implement.

ndellingwood · 2025-03-12T00:05:55Z

@lucbv some of these changes were incompatible with usage in Trilinos (intrepid2), this is showing in the nightly integration builds:

17:53:53 [ 66%] Building CXX object packages/intrepid2/unit-test/Projection/CMakeFiles/Intrepid2_unit-test_Projection_test_project_fields_Serial_DOUBLE.dir/test_project_fields_Serial_DOUBLE.cpp.o
17:53:56 In file included from /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Build/packages/intrepid2/unit-test/Projection/test_project_fields_Serial_DOUBLE.cpp:15:
17:53:56 In file included from /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Trilinos/packages/intrepid2/unit-test/Projection/test_project_fields.hpp:34:
17:53:56 /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Trilinos/packages/intrepid2/src/Projection/Intrepid2_ProjectionTools.hpp:527:9: error: no member named 'SerialQR_Internal' in namespace 'KokkosBatched'; did you mean 'KokkosBatched::Impl::SerialQR_Internal'?
17:53:56         KokkosBatched::SerialQR_Internal::invoke(A0_host.extent(0), A0_host.extent(1),
17:53:56         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17:53:56         KokkosBatched::Impl::SerialQR_Internal
17:53:56 /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Trilinos/kokkos-kernels/batched/dense/impl/KokkosBatched_QR_Serial_Internal.hpp:34:8: note: 'KokkosBatched::Impl::SerialQR_Internal' declared here
17:53:56 struct SerialQR_Internal {
17:53:56        ^
17:53:56 In file included from /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Build/packages/intrepid2/unit-test/Projection/test_project_fields_Serial_DOUBLE.cpp:15:
17:53:56 In file included from /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Trilinos/packages/intrepid2/unit-test/Projection/test_project_fields.hpp:34:
17:53:56 /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Trilinos/packages/intrepid2/src/Projection/Intrepid2_ProjectionTools.hpp:583:11: error: no member named 'SerialQR_Internal' in namespace 'KokkosBatched'; did you mean 'KokkosBatched::Impl::SerialQR_Internal'?
17:53:56           KokkosBatched::SerialQR_Internal::invoke(A.extent(0), A.extent(1),
17:53:56           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17:53:56           KokkosBatched::Impl::SerialQR_Internal
17:53:56 /home/jenkins/blake-new/workspace/KokkosEco_Trilinos_Blake_OneAPI2023_2_0_ICPX_Serial/Trilinos/kokkos-kernels/batched/dense/impl/KokkosBatched_QR_Serial_Internal.hpp:34:8: note: 'KokkosBatched::Impl::SerialQR_Internal' declared here
17:53:56 struct SerialQR_Internal {

Should I be able to add a guard on version and include updates to include the Impl namespace to resolve?

Update in Intrepid2_ProjectionTools.hpp for compatibility changes in kokkos/kokkos-kernels#2342 Signed-off-by: Nathan Ellingwood <ndellin@sandia.gov>

ndellingwood · 2025-03-12T01:02:15Z

Yeah, simple namespace update was all that was needed, PR up trilinos/Trilinos#13876

lucbv mentioned this pull request Sep 19, 2024

QR on a single matrix: valgrind reports invalid reads and writes #2328

Open

lucbv added the AT: WIP label Oct 8, 2024

lucbv force-pushed the batched_QR_fixes branch 2 times, most recently from 94496d2 to 4bfb715 Compare October 8, 2024 23:29

cwpearson requested changes Oct 10, 2024

View reviewed changes

batched/dense/impl/KokkosBatched_QR_FormQ_Serial_Internal.hpp Show resolved Hide resolved

batched/dense/unit_test/Test_Batched_SerialQR.hpp Show resolved Hide resolved

lucbv self-assigned this Oct 15, 2024

lucbv force-pushed the batched_QR_fixes branch 2 times, most recently from 18fc349 to b24686b Compare November 6, 2024 20:45

lucbv force-pushed the batched_QR_fixes branch 2 times, most recently from 2b4e03b to f4dfbe9 Compare December 4, 2024 20:39

lucbv removed the AT: WIP label Dec 4, 2024

lucbv force-pushed the batched_QR_fixes branch from cefa092 to e2771fd Compare January 21, 2025 14:22

lucbv force-pushed the batched_QR_fixes branch from 6aa3beb to f3460b6 Compare March 7, 2025 22:11

lucbv added 5 commits March 7, 2025 15:32

Removing the stride for the workspace

a6c31da

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

Applying clang-format

a880afc

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

Adding checks on inputs

a9af98f

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

More clean-ups

c67a7b0

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

clang-format

eb4c625

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

lucbv requested review from cwpearson and yasahi-hpc March 8, 2025 00:08

yasahi-hpc requested changes Mar 8, 2025

View reviewed changes

lucbv force-pushed the batched_QR_fixes branch from dd9ecda to a9b516e Compare March 10, 2025 21:11

Batched - QR: adding check for apply Q on the right side.

d921b2d

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

lucbv added bug enhancement labels Mar 10, 2025

lucbv added 2 commits March 10, 2025 17:18

clang-format

0791676

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

clang-format

c5ec7e7

Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>

lucbv requested a review from yasahi-hpc March 10, 2025 23:22

yasahi-hpc approved these changes Mar 11, 2025

View reviewed changes

cwpearson approved these changes Mar 11, 2025

View reviewed changes

lucbv merged commit d343008 into kokkos:develop Mar 11, 2025
19 checks passed

ndellingwood mentioned this pull request Mar 12, 2025

intrepid2: update SerialQR_Internal namespacing trilinos/Trilinos#13876

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batched - dense: Testing and fixing Serial QR #2342

batched - dense: Testing and fixing Serial QR #2342

lucbv commented Sep 19, 2024 •

edited

Loading

cwpearson left a comment •

edited

Loading

yasahi-hpc left a comment

lucbv commented Mar 10, 2025

yasahi-hpc left a comment

ndellingwood commented Mar 12, 2025 •

edited

Loading

ndellingwood commented Mar 12, 2025

batched - dense: Testing and fixing Serial QR #2342

batched - dense: Testing and fixing Serial QR #2342

Conversation

lucbv commented Sep 19, 2024 • edited Loading

cwpearson left a comment • edited Loading

Choose a reason for hiding this comment

yasahi-hpc left a comment

Choose a reason for hiding this comment

lucbv commented Mar 10, 2025

yasahi-hpc left a comment

Choose a reason for hiding this comment

ndellingwood commented Mar 12, 2025 • edited Loading

ndellingwood commented Mar 12, 2025

lucbv commented Sep 19, 2024 •

edited

Loading

cwpearson left a comment •

edited

Loading

ndellingwood commented Mar 12, 2025 •

edited

Loading