eth-cscs / DLA-Future

DLA-Future
https://eth-cscs.github.io/DLA-Future/master/
BSD 3-Clause "New" or "Revised" License
64 stars 14 forks source link

Enable `copy` to be used with MatrixRef #1021

Closed albestro closed 11 months ago

albestro commented 11 months ago

Basic implementation (mainly copy-paste) for a copy method involving MatrixRefs.

Currently Matrix and MatrixRef are still two independent objects, and this implies that we would have to duplicate all the code for the Matrix (more or less, e.g. MatrixRef might have an offset which creates some new corner cases) the same for MatrixRef.

This represents a problem

Nothing new, but since it does not sound useful to spend time replicating things like that, in this PR I started with just the basic implementation of the copy and as soon as we take a decision on how to proceed, I will try to apply here too. 😉

Waiting for your ideas and suggestions!

EDIT: After a discussion with others, we opted for using copy as a "testbed" also for the sub-matrix/pipeline mechanisms, as a "real" use-case. This means that test for copy will be more "extensive" wrt other algorithms (e.g. gemm) and we will have:

For the rest of the algorithms (so gemm included), we will just test the with MatrixRef both full and sub-matrices cases (where applicable).

msimberg commented 11 months ago

Just for the record, I will benchmark these changes: https://github.com/eth-cscs/DLA-Future/compare/master...msimberg:DLA-Future:matrix-ref-algorithm-input-shortcut-trivial-ref. Hopefully by early next week we'll know more about if we can possibly only instantiate algorithms with MatrixRef and then you could update this PR to do the same.

albestro commented 11 months ago

cscs-ci run

codecov-commenter commented 11 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (3097ab4) 94.10% compared to head (b1a41bf) 94.09%. Report is 10 commits behind head on master.

Files Patch % Lines
include/dlaf/matrix/copy.h 81.81% 1 Missing and 1 partial :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1021 +/- ## ========================================== - Coverage 94.10% 94.09% -0.02% ========================================== Files 146 146 Lines 8843 8856 +13 Branches 1121 1125 +4 ========================================== + Hits 8322 8333 +11 - Misses 326 327 +1 - Partials 195 196 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

albestro commented 11 months ago

cscs-ci run

rebased on new master (for getting matrix base snake_case) and addressed snake_case in implementation too.

after this it is ready to be merged 👍🏻