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

Add tensor type to cudaq. #2304

Open
wants to merge 78 commits into
base: main
Choose a base branch
from
Open

Add tensor type to cudaq. #2304

wants to merge 78 commits into from

Conversation

amccaskey
Copy link
Collaborator

No description provided.

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 21, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@amccaskey amccaskey marked this pull request as draft October 21, 2024 14:04
@schweitzpgi
Copy link
Collaborator

@anthony-santana @sacpis Is this the interface you were looking for? (makes Jedi hand motion.)

anthony-santana
anthony-santana reviewed Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@anthony-santana anthony-santana left a comment •
edited
Loading

Choose a reason for hiding this comment

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

EDIT: removed outdated comment

@amccaskey
add cudaq tensor type
6f8eeb2 
Signed-off-by: Alex McCaskey <[email protected]>
@amccaskey
add uint8 tensor to python api
02dff59 
Signed-off-by: Alex McCaskey <[email protected]>
schweitzpgi
schweitzpgi reviewed Oct 22, 2024
View reviewed changes
python/runtime/utils/py_tensor.cpp Show resolved Hide resolved
runtime/cudaq/utils/details/impls/xtensor_impl.cpp Outdated Show resolved Hide resolved
runtime/cudaq/utils/details/impls/xtensor_impl.cpp Outdated Show resolved Hide resolved
runtime/cudaq/utils/details/impls/xtensor_impl.cpp Show resolved Hide resolved
runtime/cudaq/utils/details/impls/xtensor_impl.cpp Outdated Show resolved Hide resolved
runtime/cudaq/utils/tensor.h Outdated Show resolved Hide resolved
runtime/cudaq/utils/type_traits.h Show resolved Hide resolved
unittests/utils/tensor_tester.cpp Outdated Show resolved Hide resolved
unittests/utils/tensor_tester.cpp Outdated Show resolved Hide resolved
unittests/utils/tensor_tester.cpp Outdated Show resolved Hide resolved
@schweitzpgi
Copy link
Collaborator

schweitzpgi commented Oct 22, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

schweitzpgi and others added 11 commits October 22, 2024 13:15
@schweitzpgi
Update unittests/utils/tensor_tester.cpp
94317a7 
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi
Update unittests/utils/tensor_tester.cpp
f958c3c 
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi
Update unittests/utils/tensor_tester.cpp
a432b54 
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi
Update runtime/cudaq/utils/details/impls/xtensor_impl.cpp
6f9afee 
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi
Update runtime/cudaq/utils/details/impls/xtensor_impl.cpp
564820d 
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi
Update runtime/cudaq/utils/details/impls/xtensor_impl.cpp
e6e9e17 
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi
Update python/runtime/utils/py_tensor.cpp
1328585 
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi @bmhowe23
Update runtime/cudaq/utils/extension_point.h
728a10d 
Co-authored-by: Ben Howe <[email protected]>
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi @bmhowe23
Update runtime/cudaq/utils/extension_point.h
048420f 
Co-authored-by: Ben Howe <[email protected]>
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi @bmhowe23
Update runtime/cudaq/utils/tensor.h
16d35ac 
Co-authored-by: Ben Howe <[email protected]>
Signed-off-by: Eric Schweitz <[email protected]>
@schweitzpgi @bmhowe23
Update runtime/cudaq/utils/tensor.h
1a9bd72 
Co-authored-by: Ben Howe <[email protected]>
Signed-off-by: Eric Schweitz <[email protected]>
@annagrin
Copy link
Collaborator

annagrin commented Oct 24, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@annagrin
Copy link
Collaborator

annagrin commented Oct 24, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@annagrin
Remove temp file
3a58eea 
Signed-off-by: Anna Gringauze <[email protected]>
@annagrin
Copy link
Collaborator

annagrin commented Oct 24, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@annagrin
Copy link
Collaborator

annagrin commented Oct 25, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@annagrin
Copy link
Collaborator

annagrin commented Oct 25, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

annagrin
annagrin reviewed Oct 25, 2024
View reviewed changes
unittests/utils/tensor_tester.cpp
throw std::runtime_error(info->name());
}

EXPECT_THROW(t.at({2, 0}), std::runtime_error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test fails in CI Create CUDA Quantum installer (xxx) / Build CUDA Quantum assets task

Copy link
Collaborator

Choose a reason for hiding this comment

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

Trying to disable.

@annagrin
Copy link
Collaborator

annagrin commented Oct 25, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@amccaskey
Copy link
Collaborator Author

Before merging this, you should add a test (and get it working) for using cudaq::tensor in an application file compiled with nvq++. There are issues with regards to compiler compatibility and extension_point static initialization that may show up there. May need to remove extension_point and just use xtensor directly.

@annagrin
Copy link
Collaborator

annagrin commented Oct 25, 2024 •
edited by github-actions bot
Loading

/ok to test

Command Bot: Processing...

@annagrin
Copy link
Collaborator

Before merging this, you should add a test (and get it working) for using cudaq::tensor in an application file compiled with nvq++. There are issues with regards to compiler compatibility and extension_point static initialization that may show up there. May need to remove extension_point and just use xtensor directly.

I gave it a try in amccaskey#6, let me know if this is in the right direction.

amccaskey
amccaskey commented Oct 26, 2024
View reviewed changes
runtime/cudaq/utils/details/tensor_impl.h
dd_add(const tensor_impl<Scalar> &left) const = 0;

// Terminal implementation of operators.
virtual tensor_impl<Scalar> *multiply(const xtensor<Scalar> &right) const = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is the use of xtensor intended here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. This is using the old-school double dispatch pattern to determine the two input tensors have the same implementation. This is needed so that our matrix multiply code can efficiently (and correctly) access both of the input matrices to compute the product.

I'm thinking this implementation hierarchy ought to get rid of the virtual methods and be redone using the curiously recurring template pattern. However, I left it as it was to this point.

@schweitzpgi schweitzpgi mentioned this pull request Oct 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmhowe23 bmhowe23 bmhowe23 left review comments

@anthony-santana anthony-santana anthony-santana left review comments

@schweitzpgi schweitzpgi schweitzpgi left review comments

@annagrin annagrin annagrin left review comments

@bettinaheim bettinaheim Awaiting requested review from bettinaheim bettinaheim is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amccaskey @schweitzpgi @annagrin @anthony-santana @bmhowe23