Conversation
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
kshyatt
force-pushed
the
ksh/cu
branch
4 times, most recently
from
046f4d0 to
44d6fe6
Compare
kshyatt
force-pushed
the
ksh/cu
branch
2 times, most recently
from
11674d6 to
d4e1a03
Compare
|
Had to rebase on top of |
|
(just converted to draft to avoid rerunning all the tests for the various rebases) |
Oh good call, thank you! |
|
FWIW all this is why I normally don't cherry-pick off other branches 😇 |
kshyatt
force-pushed
the
ksh/cu
branch
2 times, most recently
from
648699a to
64bf622
Compare
|
OK, I think everything here got addressed, should I put this on R4R and run the full tests? |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
I think overall this looks good to go, except for the fact that I don't think it makes sense to include all examples for a GPU version as well.
I do agree that there must be examples about GPU support, but I don't think it makes much sense to translate all of them, since we then also have to maintain all of them 🙃
What I would like instead is for the tests to include some more GPU cases, but I guess that is a way bigger endeavor so I also am happy to just include the changes without that for now
|
I 100% agree about the examples, IMO let me cleanup the multiline stuff a little and try to write a few tests that will actually cover the changes. |
kshyatt
force-pushed
the
ksh/cu
branch
3 times, most recently
from
35f3276 to
f83f282
Compare
|
Some of the missing coverage here will be fixed once the Algorithm and Driver changes hit at MatrixAlgebraKit, I'll try to add a few more tests for the MPOs though |
|
Some of this needs a fix in TensorOperations, which I'm working on, but I think this can be merged and I'll come back and re-enable the tests affected by |
Fix sources Don't use a storagetype as input for MPS's Remove duplicates in extension more comments Apply suggestions from code review Co-authored-by: Lukas Devos <ldevos98@gmail.com> Fix sources Remove some extra storagetypes
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Overall I definitely think we've been going over this for long enough, most of it looks very reasonable to me. I left some final small comments, but otherwise I'd be happy to merge this and continue from there.
2 participants
Mostly allowing passing an array type instead of just an element type to allow GPU arrays to back MPS tensors