Compare TUV-X Tridiagonal Solver Code with LAPACKE's implementation #97

AdityaDendukuri · 2024-06-24T21:23:02Z

Tests involved generating random matrices in both single and double precision across various sizes (ranging from 1000 to 10,000,000).
Added a benchmarking directory in the project that uses google benchmark for evaluating runtimes for solver code.

Closes #91

…he metrics being recorded

test/unit/linear_algebra/test_lapacke.cpp

include/tuvx/linear_algebra/banded_matrix.hpp

tool/numerical_analysis/tridiagonal_solver/double.png

K20shores

The uni tests for the tridiagonal solver seem to contain benchmarking code. I don't think that belongs in the unit tests. I also question if we really want all of the stuff added in tool. It's good work, but maybe it belongs in a separate repository

CMakeLists.txt

cmake/test_util.cmake

include/tuvx/linear_algebra/linear_algebra.inl

src/CMakeLists.txt

test/unit/linear_algebra/test_lapacke.cpp

formatting changes Co-authored-by: Kyle Shores <[email protected]>

benchmark/benchmark_tridiagonal_solver.cpp

docker/Dockerfile

K20shores · 2024-07-03T20:53:43Z

cmake/FindLAPACKE.cmake

We are fine to include and distribute this if we need to.

To do so, we need to include in our release notes and anytime we mention our licensing that most of tuvx is licensed under Apache 2.0 with copyright attributed to NCAR, except for this specific file.

@mattldawson @sjsprecious @AdityaDendukuri

Thanks for figuring it out. That seems fine to me.

cmake/test_util.cmake

CMakeLists.txt

boulderdaze

Thank you for addressing the comments! I commented about the comment style. Could you review the style in the comments in this PR and make adjustments if you need? Thank you!

include/tuvx/linear_algebra/linear_algebra.hpp

boulderdaze

Thanks for addressing the comments!

mattldawson

looks great to me! just a couple minor suggestions

mattldawson · 2024-07-08T19:51:09Z

cmake/dependencies.cmake

@@ -6,6 +6,7 @@ include(FetchContent)

 find_package(BLAS)
 find_package(LAPACK)
+find_package(LAPACKE)


should this go inside the if (TUVX_ENABLE_BENCHMARK) block? Is it only needed for the benchmarking tests?

@mattldawson @sjsprecious @K20shores LAPACKE is also used in tests. Since LAPACKE does its own tests when included in CMAKE, is it a good idea to remove the unit tests I wrote for LAPACKE?

I think it's fine to include your tests - the find_pacakge() doesn't need to be in the if (TUVX_ENABLE_BENCHMARK) block. I just thought if it was only used for the benchmarking, it could be in that block, but it's ok where it as, too (in my opinion).

src/CMakeLists.txt

test/unit/linear_algebra/CMakeLists.txt

Co-authored-by: Kyle Shores <[email protected]>

codecov-commenter · 2024-07-09T15:27:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.27%. Comparing base (48f3aa5) to head (38e9903).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
+ Coverage   79.21%   79.27%   +0.06%     
==========================================
  Files         133      134       +1     
  Lines       11193    11284      +91     
==========================================
+ Hits         8866     8945      +79     
- Misses       2327     2339      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mwaxmonsky

Great work @AdityaDendukuri! Just a few questions/recommendations but otherwise looks good to me!

mwaxmonsky · 2024-07-09T20:33:07Z

docker/Dockerfile.docs

+      && cmake \
+        -D TUVX_ENABLE_TESTS=OFF \
+        -D TUVX_ENABLE_BENCHMARK=OFF \
+        -D TUVX_BUILD_DOCS=ON \
+        /tuv-x \


Can this be reverted since TUVX_ENABLE_BENCHMARK is defaulted to OFF?

mwaxmonsky · 2024-07-10T12:58:56Z

.github/workflows/ubuntu.yml

-      run: cmake --build build --parallel
+      run: cmake --build build 


I may have missed this part of the conversation so my apologies in advanced but is --parallel not possible to use with lapacke

mwaxmonsky · 2024-07-10T13:01:00Z

docker/Dockerfile

-COPY . /tuv-x/
-RUN mkdir /build \
-      && cd /build \
+COPY . /tuv-x
+RUN cd /tuv-x \
+      && mkdir build \
+      && cd build \
      && cmake -D CMAKE_BUILD_TYPE=release \
               -D TUVX_INSTALL_INCLUDE_DIR=/usr/local/include \
               -D TUVX_INSTALL_MOD_DIR=/usr/local/include \
-               /tuv-x \
+               .. \
      && make install -j 8

-WORKDIR /build
+WORKDIR /tuv-x/build


Can these changes be reverted to match the rest of the Dockerfile's or is this change still needed?

src/CMakeLists.txt

Co-authored-by: mwaxmonsky <[email protected]>

sjsprecious

Thanks @AdityaDendukuri for working on this PR and addressing all the comments.

Aditya Dendukuri and others added 7 commits May 22, 2024 13:51

added BLAS library specification in build script

0700222

merged local branch with main

6fee119

Merge branch 'main' of github.com:NCAR/tuv-x

0db8fa3

lapacke added in cmake

42d8147

added google benchmark

c2d6472

run each tridiagonal solver test multiple times and take average of t…

c74f3f2

…he metrics being recorded

modified plots generated in numerical_analysis

bdcb3a0

AdityaDendukuri requested review from sjsprecious, mattldawson, K20shores, boulderdaze and mwaxmonsky June 24, 2024 21:23

sjsprecious reviewed Jun 25, 2024

View reviewed changes

test/unit/linear_algebra/test_lapacke.cpp Outdated Show resolved Hide resolved

test/unit/linear_algebra/test_lapacke.cpp Outdated Show resolved Hide resolved

include the correct LAPACK package based on the compiler used

b1b5ff4

AdityaDendukuri requested a review from sjsprecious June 26, 2024 17:37

sjsprecious reviewed Jun 26, 2024

View reviewed changes

include/tuvx/linear_algebra/banded_matrix.hpp Outdated Show resolved Hide resolved

sjsprecious reviewed Jun 26, 2024

View reviewed changes

tool/numerical_analysis/tridiagonal_solver/double.png Outdated Show resolved Hide resolved

K20shores reviewed Jun 26, 2024

View reviewed changes

AdityaDendukuri and others added 2 commits June 26, 2024 13:18

Apply suggestions from code review

dee5d7b

formatting changes Co-authored-by: Kyle Shores <[email protected]>

seperated tests and benchmark (used google benchmark)

830184d

AdityaDendukuri requested review from sjsprecious and K20shores June 27, 2024 16:13

sjsprecious requested changes Jun 28, 2024

View reviewed changes

benchmark/benchmark_tridiagonal_solver.cpp Outdated Show resolved Hide resolved

benchmark/benchmark_tridiagonal_solver.cpp Outdated Show resolved Hide resolved

Aditya Dendukuri and others added 4 commits June 28, 2024 11:21

benchmarking code changes

5e63955

only use diagonally dominant matrices for unit test

3cbb75a

Merge branch 'main' into lapacke_debug

2077a2d

update docker files to use lapacke for tridiagonal tests

3acc6e4

sjsprecious reviewed Jun 28, 2024

View reviewed changes

docker/Dockerfile Outdated Show resolved Hide resolved

sjsprecious added 2 commits June 28, 2024 17:21

Update ubuntu.yml with lapacke

e035ad9

maybe this will work?

cc3403b

Aditya Dendukuri and others added 2 commits July 3, 2024 11:51

Merge branch 'lapacke_debug' of github.com:NCAR/tuv-x into lapacke_debug

9f256bd

reverted formatting changes in cmake file

bb1c227

K20shores reviewed Jul 3, 2024

View reviewed changes

sjsprecious reviewed Jul 3, 2024

View reviewed changes

cmake/test_util.cmake Outdated Show resolved Hide resolved

sjsprecious reviewed Jul 3, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Aditya Dendukuri added 3 commits July 3, 2024 15:18

re-added lapacke dependency in test_util

05e2827

linked LAPACKE libraries to unit tests

04a9b0f

removed typedefs in test_error_function.cpp

20e73de

AdityaDendukuri requested a review from boulderdaze July 3, 2024 22:12

boulderdaze reviewed Jul 5, 2024

View reviewed changes

include/tuvx/linear_algebra/linear_algebra.hpp Outdated Show resolved Hide resolved

updated code documentation

34b529b

AdityaDendukuri requested a review from boulderdaze July 5, 2024 21:42

AdityaDendukuri and others added 4 commits July 5, 2024 16:47

documentaion changes

88fa5a6

documentation changes

302fc37

documentation changes

73ac682

switched back to version 0.9.0

1d165a8

AdityaDendukuri requested a review from sjsprecious July 8, 2024 15:01

Documentation Changes

23b358e

boulderdaze approved these changes Jul 8, 2024

View reviewed changes

mattldawson approved these changes Jul 9, 2024

View reviewed changes

AdityaDendukuri and others added 2 commits July 9, 2024 09:22

Update src/CMakeLists.txt

a572368

Co-authored-by: Kyle Shores <[email protected]>

Update src/CMakeLists.txt

38e9903

Co-authored-by: Kyle Shores <[email protected]>

Aditya Dendukuri added 2 commits July 9, 2024 09:36

rolled back format changes

b6a10f8

Merge branch 'lapacke_debug' of github.com:NCAR/tuv-x into lapacke_debug

766ecf8

mwaxmonsky approved these changes Jul 10, 2024

View reviewed changes

Update src/CMakeLists.txt

98d29f6

Co-authored-by: mwaxmonsky <[email protected]>

sjsprecious approved these changes Jul 11, 2024

View reviewed changes

mattldawson merged commit 9b48b5d into main Jul 16, 2024
10 checks passed

mattldawson deleted the lapacke_debug branch July 16, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compare TUV-X Tridiagonal Solver Code with LAPACKE's implementation #97

Compare TUV-X Tridiagonal Solver Code with LAPACKE's implementation #97

AdityaDendukuri commented Jun 24, 2024 •

edited

Loading

K20shores left a comment

K20shores Jul 3, 2024

sjsprecious Jul 3, 2024

boulderdaze left a comment

boulderdaze left a comment

mattldawson left a comment

mattldawson Jul 8, 2024

AdityaDendukuri Jul 9, 2024

mattldawson Jul 9, 2024

codecov-commenter commented Jul 9, 2024

mwaxmonsky left a comment

mwaxmonsky Jul 9, 2024

mwaxmonsky Jul 10, 2024

mwaxmonsky Jul 10, 2024

sjsprecious left a comment

Compare TUV-X Tridiagonal Solver Code with LAPACKE's implementation #97

Compare TUV-X Tridiagonal Solver Code with LAPACKE's implementation #97

Conversation

AdityaDendukuri commented Jun 24, 2024 • edited Loading

K20shores left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boulderdaze left a comment

Choose a reason for hiding this comment

boulderdaze left a comment

Choose a reason for hiding this comment

mattldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 9, 2024

Codecov Report

mwaxmonsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjsprecious left a comment

Choose a reason for hiding this comment

AdityaDendukuri commented Jun 24, 2024 •

edited

Loading