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

Refactor RetiledMatrix into Matrix #935

Merged
merged 5 commits into from
Jul 19, 2023

Conversation

msimberg
Copy link
Collaborator

@msimberg msimberg commented Jul 12, 2023

Fixes #905.

I moved the creation of a RetiledMatrix into Matrix methods retiledSubPipeline{,Const} to mirror #898. Since the implementation is otherwise identical I've kept the "sub pipeline" name, but added retiled as a prefix (though the naming concerns from #898 apply here as well). I've removed the RetiledMatrix class and replaced it with Matrix. I've added an assertion to only allow retiling if the Matrix isn't already "retiled", i.e. if the tile size is the same as the block size. We can attempt to lift this restriction later if it's useful but I won't be doing that here. #923 also applies for retiled matrices and I'm not doing that in this PR either.

This should probably wait for #933 to be merged first, or maybe not...

@msimberg msimberg added this to the API refactoring milestone Jul 12, 2023
@msimberg msimberg self-assigned this Jul 12, 2023
@msimberg
Copy link
Collaborator Author

cscs-ci run

@msimberg
Copy link
Collaborator Author

cscs-ci run

Copy link
Collaborator

@rasolca rasolca left a comment

Choose a reason for hiding this comment

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

I have only a concern.
Algorithm implementations might not work with a retiled matrix as argument.

Therefore for each function that takes a matrix as argument we should either enhance the test and make sure it works or add an assertion.

Of course updating all the tests and fixing the algorithms does not belong to this PR.

Should we add an assertion at least in the public algorithms until they are tested and known to work?

Note: as a retiled matrix is now a Matrix, retiled of retiled will be needed. (e.g for complete functionality of #933)

@msimberg
Copy link
Collaborator Author

I have only a concern. Algorithm implementations might not work with a retiled matrix as argument.

Good point.

Therefore for each function that takes a matrix as argument we should either enhance the test and make sure it works or add an assertion.

Of course updating all the tests and fixing the algorithms does not belong to this PR.

Should we add an assertion at least in the public algorithms until they are tested and known to work?

I think that would be good. I will add that here.

Note: as a retiled matrix is now a Matrix, retiled of retiled will be needed. (e.g for complete functionality of #933)

Yeah, with the discussion in #933 I would include the redistributing copy to the list of algorithms above that currently require same block size and tile size. Then we can case by case relax the requirements/test what works with retiled matrices.

@msimberg
Copy link
Collaborator Author

msimberg commented Jul 13, 2023

@rasolca, @albestro see 0f5a091 for assertions on algorithms.

I added a helper function which I now called retiled(MatrixLike). I'm not super happy with the name, but it was better than unretiled. I could go for a more explicit equal_tile_and_block(_size) or something in that direction as well. Suggestions welcome.

@rasolca
Copy link
Collaborator

rasolca commented Jul 13, 2023

What about single_tile_per_block(matrix)?

@albestro
Copy link
Collaborator

I like both retiled and single_tile_per_block 😉 up to you. Thanks for adding the helper 👍🏻

@msimberg
Copy link
Collaborator Author

I'll go with single_tile_per_block. Thanks for the comments! Pushing soon...

@msimberg
Copy link
Collaborator Author

See f3ecb55 for the renaming to single_tile_per_block. Would you prefer if I update the preconditions in the docs from

@pre mat has equal tile and block sizes

to

@pre mat has a single tile per block

to match?

@rasolca
Copy link
Collaborator

rasolca commented Jul 14, 2023

See f3ecb55 for the renaming to single_tile_per_block. Would you prefer if I update the preconditions in the docs from

@pre mat has equal tile and block sizes

to

@pre mat has a single tile per block

to match?

No strong preference on my side.

include/dlaf/multiplication/general.h Outdated Show resolved Hide resolved
include/dlaf/eigensolver/eigensolver.h Outdated Show resolved Hide resolved
include/dlaf/permutations/general.h Outdated Show resolved Hide resolved
@msimberg
Copy link
Collaborator Author

cscs-ci run

@msimberg msimberg requested a review from albestro July 19, 2023 11:09
@rasolca rasolca merged commit 0873b9d into eth-cscs:master Jul 19, 2023
github-actions bot pushed a commit that referenced this pull request Jul 19, 2023
@msimberg msimberg deleted the retiled-matrix-as-matrix branch July 20, 2023 07:27
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.

Change RetiledMatrix to Matrix
3 participants