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

add miniapp triangular multiplication #1029

Merged
merged 17 commits into from
Dec 12, 2023

Conversation

gulivarese
Copy link
Collaborator

@gulivarese gulivarese commented Nov 6, 2023

Fixes #763.

@gulivarese gulivarese marked this pull request as draft November 6, 2023 12:36
@gulivarese gulivarese changed the title Guli/miniapp triangular multiplication add miniapp triangular multiplication Nov 6, 2023
@gulivarese gulivarese force-pushed the guli/miniapp_triangular_multiplication branch from aff92d6 to 4f16773 Compare November 8, 2023 10:54
Guglielmo Gagliardi added 2 commits November 8, 2023 12:02
@gulivarese gulivarese force-pushed the guli/miniapp_triangular_multiplication branch from 4f16773 to 29b9dc5 Compare November 8, 2023 11:14
@gulivarese gulivarese marked this pull request as ready for review November 28, 2023 10:00
scripts/miniapps.py Outdated Show resolved Hide resolved
scripts/postprocess.py Outdated Show resolved Hide resolved
miniapp/miniapp_triangular_multiplication.cpp Outdated Show resolved Hide resolved
miniapp/miniapp_triangular_multiplication.cpp Outdated Show resolved Hide resolved
Comment on lines +240 to +241
template <typename T>
linear_system_t<T> sampleLeftTr(blas::Uplo uplo, blas::Op op, blas::Diag diag, T alpha, SizeType m) {
Copy link
Collaborator

@rasolca rasolca Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: This is code duplication, but it will disappear in favor of random matrices to be able to:

  • re-enable tests
  • allow complex types

@rasolca
Copy link
Collaborator

rasolca commented Nov 28, 2023

cscs-ci run

1 similar comment
@gulivarese
Copy link
Collaborator Author

cscs-ci run

@gulivarese
Copy link
Collaborator Author

cscs-ci run

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e203634) 94.10% compared to head (3fc72f4) 94.10%.
Report is 28 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1029   +/-   ##
=======================================
  Coverage   94.10%   94.10%           
=======================================
  Files         146      146           
  Lines        8843     8843           
  Branches     1121     1121           
=======================================
  Hits         8322     8322           
  Misses        326      326           
  Partials      195      195           

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

miniapp/miniapp_triangular_multiplication.cpp Show resolved Hide resolved
DLAF_MPI_CHECK_ERROR(MPI_Barrier(world));
};

const auto side = opts.side;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to assert the value of this parameter, given that we have sampleLeftTr?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is needed here and in the triangular solver miniapp as well. Currently the matrix a size is (m x m), so Side::Right fails unless m == n.

miniapp/miniapp_triangular_multiplication.cpp Outdated Show resolved Hide resolved
miniapp/miniapp_triangular_multiplication.cpp Outdated Show resolved Hide resolved
miniapp/miniapp_triangular_multiplication.cpp Outdated Show resolved Hide resolved
@gulivarese
Copy link
Collaborator Author

cscs-ci run

@gulivarese
Copy link
Collaborator Author

cscs-ci run

const auto side = opts.side;
DLAF_ASSERT(side == blas::Side::Left, side);

const auto side = opts.side;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This redefines side (already defined above). See CI falure.

@gulivarese
Copy link
Collaborator Author

cscs-ci run

@gulivarese
Copy link
Collaborator Author

cscs-ci run

@gulivarese
Copy link
Collaborator Author

cscs-ci run

@gulivarese
Copy link
Collaborator Author

cscs-ci run

@rasolca rasolca merged commit 72b5210 into master Dec 12, 2023
4 checks passed
@rasolca rasolca deleted the guli/miniapp_triangular_multiplication branch December 12, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Miniapp triangular multiplication
4 participants