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.

Sign up for GitHub

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

Jump to bottom

Refactor distribution and extend tests for sub-distributions #976

Merged
merged 16 commits into from
Oct 13, 2023
Merged

Refactor distribution and extend tests for sub-distributions #976

merged 16 commits into from
Oct 13, 2023

Conversation

rasolca
Copy link
Collaborator

@rasolca rasolca commented Sep 7, 2023 •
edited
Loading

This PR consists of

  • util_distribution.h
    • refactoring
    • extend tests with offsets
      • fix a couple of bugs when using offsets
  • distribution.h
    • refactoring
    • extract specific methods to distribution_extensions.h (internal methods)
    • create aliases with the old names (which can be deprecated)
    • extend tests
      • fix bugs (Note: some bugs already fixed but not tested.)
    • implement new methods needed for the distributed tridiagonal solver in distribution_extensions.h

@rasolca rasolca added this to the API refactoring milestone Sep 7, 2023
@rasolca rasolca self-assigned this Sep 7, 2023
@rasolca
refactor distribution (keeping alias with old names), add flag to dep…
b5e4dbc 
…recate old names, move specific methods to distribution_extensions.h, disable tests for methods that do not exists anymore
@rasolca rasolca force-pushed the rasolca/distribution/cleanup branch from 4ee8411 to b5e4dbc Compare September 7, 2023 14:16
msimberg
msimberg reviewed Sep 7, 2023
View reviewed changes
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.

It's a bit hard to follow the changes because of the renamings, but from what I can tell it looks all valid. Thanks for spotting these issues!

I only have a few higher level comments at this point.

@rasolca
Copy link
Collaborator Author

rasolca commented Sep 8, 2023

It's a bit hard to follow the changes because of the renamings, but from what I can tell it looks all valid. Thanks for spotting these issues!

I only have a few higher level comments at this point.

Sorry, the first review was only meant to check if the new interface made sense.

@rasolca rasolca force-pushed the rasolca/distribution/cleanup branch from 101e069 to 3e79dd8 Compare September 27, 2023 19:00
@rasolca rasolca force-pushed the rasolca/distribution/cleanup branch from 3e79dd8 to 37a0968 Compare September 27, 2023 19:10
@rasolca
Copy link
Collaborator Author

rasolca commented Sep 27, 2023

cscs-ci run

@rasolca
Copy link
Collaborator Author

rasolca commented Sep 27, 2023

cscs-ci run

@rasolca
Copy link
Collaborator Author

rasolca commented Sep 28, 2023

cscs-ci run

@rasolca rasolca marked this pull request as ready for review September 28, 2023 13:31
@rasolca
Copy link
Collaborator Author

rasolca commented Sep 28, 2023

cscs-ci run

@rasolca
Copy link
Collaborator Author

rasolca commented Sep 28, 2023

cscs-ci run

@rasolca rasolca marked this pull request as draft September 28, 2023 15:38
@rasolca
renamed global_nr_tiles_ to nr_tiles_ to match with the getter, impro…
ce0b617 
…ved the doc, and fixed calculation of local sizes in the constructor when offset != 0 and add extra tests
@rasolca
Copy link
Collaborator Author

rasolca commented Sep 29, 2023

cscs-ci run

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2023 •
edited
Loading

Codecov Report

Merging #976 (bcb580a) into master (52704e9) will increase coverage by 0.16%.
Report is 28 commits behind head on master.
The diff coverage is 94.18%.

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

@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
+ Coverage   93.42%   93.59%   +0.16%     
==========================================
  Files         143      144       +1     
  Lines        8686     8852     +166     
  Branches     1118     1119       +1     
==========================================
+ Hits         8115     8285     +170     
+ Misses        383      374       -9     
- Partials      188      193       +5
Files Coverage Δ
include/dlaf/matrix/layout_info.h 97.61% <100.00%> (ø)
include/dlaf/matrix/util_distribution.h 80.00% <100.00%> (-0.27%) ⬇️
src/matrix/distribution.cpp 98.95% <100.00%> (+3.63%) ⬆️
include/dlaf/matrix/distribution.h 93.26% <95.05%> (+4.64%) ⬆️
include/dlaf/matrix/distribution_extensions.h 88.88% <88.88%> (ø)

... and 6 files with indirect coverage changes

@rasolca rasolca marked this pull request as ready for review September 29, 2023 12:55
msimberg
msimberg approved these changes Sep 29, 2023
View reviewed changes
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.

This looks good to me! Just a couple of general comments, not blocking.

@rasolca
Copy link
Collaborator Author

rasolca commented Sep 29, 2023

cscs-ci run

@rasolca
Copy link
Collaborator Author

rasolca commented Oct 9, 2023

cscs-ci run

@rasolca rasolca mentioned this pull request Oct 9, 2023
Merged
albestro
albestro approved these changes Oct 13, 2023
View reviewed changes
Copy link
Collaborator

@albestro albestro left a comment

Choose a reason for hiding this comment

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

Had a quick look, LGTM.

@rasolca
Copy link
Collaborator Author

rasolca commented Oct 13, 2023

cscs-ci run

@rasolca rasolca merged commit a0c5133 into master Oct 13, 2023
@rasolca rasolca deleted the rasolca/distribution/cleanup branch October 13, 2023 15:06
github-actions bot pushed a commit that referenced this pull request Oct 13, 2023
@rasolca
Doc Refactor distribution and extend tests for sub-distributions (#976)
f6e391a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msimberg msimberg msimberg approved these changes

@albestro albestro albestro approved these changes

@RMeli RMeli Awaiting requested review from RMeli

@aurianer aurianer Awaiting requested review from aurianer

Assignees

@rasolca rasolca

Labels
Priority:High Type:Cleanup Type:New Feature New feature or request Type:Refactoring
Projects
DLA-F Planning
Archived in project
Milestone
API refactoring
Development

Successfully merging this pull request may close these issues.
4 participants
@rasolca @codecov-commenter @msimberg @albestro