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

Reimplementation of local band-to-tridiagonal #938

Merged
merged 9 commits into from
Jul 21, 2023

Conversation

rasolca
Copy link
Collaborator

@rasolca rasolca commented Jul 14, 2023

Implementation without let_value.
To me it looks simpler and more readable compared to other attempt with let_value (see #939), but introduce a little trick with a shared_ptr to allow to share the vector of tiles.

The new implementation is 45-50% faster (tested 20k, 30k and 40k).

@rasolca rasolca requested a review from msimberg July 14, 2023 17:19
@msimberg
Copy link
Collaborator

Nice @rasolca! Any idea about the scaling so far (I'm just eager to hear the results, don't worry if you don't have that yet)?

For the let_value version, would you mind opening a PR with it (even if we end up closing it in favour of this one)? It's easier to compare and comment in a PR.

@rasolca
Copy link
Collaborator Author

rasolca commented Jul 17, 2023

Nice @rasolca! Any idea about the scaling so far (I'm just eager to hear the results, don't worry if you don't have that yet)?

Not yet... still trying to make the local let_value version working.

For the let_value version, would you mind opening a PR with it (even if we end up closing it in favour of this one)? It's easier to compare and comment in a PR.

#939

@rasolca
Copy link
Collaborator Author

rasolca commented Jul 19, 2023

cscs-ci run

@rasolca rasolca marked this pull request as ready for review July 19, 2023 13:26
@rasolca rasolca self-assigned this Jul 20, 2023
include/dlaf/eigensolver/band_to_tridiag/mc.h Show resolved Hide resolved
include/dlaf/eigensolver/band_to_tridiag/mc.h Outdated Show resolved Hide resolved
include/dlaf/eigensolver/band_to_tridiag/mc.h Outdated Show resolved Hide resolved
include/dlaf/eigensolver/band_to_tridiag/mc.h Outdated Show resolved Hide resolved
include/dlaf/eigensolver/band_to_tridiag/mc.h Outdated Show resolved Hide resolved
include/dlaf/eigensolver/band_to_tridiag/mc.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Approved modulo the formatting violations.

@rasolca
Copy link
Collaborator Author

rasolca commented Jul 21, 2023

cscs-ci run

@codecov-commenter
Copy link

Codecov Report

Merging #938 (188f562) into master (3d86f2b) will increase coverage by 1.53%.
The diff coverage is 98.62%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   93.20%   94.73%   +1.53%     
==========================================
  Files         136      122      -14     
  Lines        8327     7504     -823     
  Branches     1081     1023      -58     
==========================================
- Hits         7761     7109     -652     
+ Misses        391      240     -151     
+ Partials      175      155      -20     
Impacted Files Coverage Δ
include/dlaf/eigensolver/band_to_tridiag/mc.h 98.08% <98.57%> (+0.44%) ⬆️
include/dlaf/eigensolver/band_to_tridiag.h 51.51% <100.00%> (ø)
include/dlaf/eigensolver/eigensolver/impl.h 77.77% <100.00%> (ø)

... and 34 files with indirect coverage changes

@rasolca rasolca merged commit f78e3d9 into master Jul 21, 2023
@rasolca rasolca deleted the rasolca/band_to_trid_sp branch July 21, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants