-
Notifications
You must be signed in to change notification settings - Fork 15
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
UCCSD operator pool correction and integration in adapt simulator #65
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: kvmto <[email protected]>
Signed-off-by: kvmto <[email protected]>
libs/solvers/lib/operators/operator_pools/uccsd_operator_pool.cpp
Outdated
Show resolved
Hide resolved
Let's add a test to |
Signed-off-by: kvmto <[email protected]>
Signed-off-by: kvmto <[email protected]>
I think I'm missing something ... why we are updating the generic UCCSD operator pools from real to imaginary to accommodate a single assumption in adapt_simulator.cpp? Are any docs updates needed for this change? |
The uccsd operator pool is correct. However, it might not be a good idea to multiply the imaginary number "i" by the uccsd operator pool. It will not make it generic. We might use this uccsd operator pool with other algorithm such as GQE and we will need to modify this again. I would suggest multiply the imaginary number "i" somewhere within the adapt-vqe algorithm. In addition, I would suggest to run the adapt-vqe for these two geometry with sto3g basis.
We need to make sure that it work for something beyond the simple H2 molecule with 4 qubits and 3 operator pools. |
### Fix some issues to allow compilation of `uccsd` - Fix signed int overflow in `uccsd` - allow specifying shots in `vqe` Towards: NVIDIA/cuda-quantum#2357 --------- Signed-off-by: Anna Gringauze <[email protected]>
Signed-off-by: kvmto <[email protected]>
Signed-off-by: kvmto <[email protected]>
Signed-off-by: kvmto <[email protected]>
throw std::runtime_error("Invalid adapt input, operator pool is empty."); | ||
|
||
std::vector<spin_op> pool = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of making this copy, could you not retain the code that was there and just update the commutator creation to
std::complex<double> j = 1.;
if (isImaginary) j = std::complex<double>{0.0, 1.0};
commutators.push_back(H * j * pool[i] - j * pool[i] * H);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 159 (auto op = pool[maxOpIdx];), the code selects the best operator and manipulates it. We could assume that each operator in the pool contains either only real or only imaginary coefficients, in which case we would only need to perform the check on the first item. Alternatively, if we can’t make that assumption, we would need to store the result of the check for each operator and provide it in line 159. The copy is just a proposal to accommodate different assumptions. I would be happy with either.
libs/solvers/lib/stateprep/uccsd.cpp
Outdated
for (std::size_t q = p + 1; q < numOccAlpha; q++) | ||
for (std::size_t r = 0; r < numVirtAlpha - 1; r++) | ||
for (std::size_t s = r + 1; s < numVirtAlpha; s++) { | ||
for (int p = 0; p < numOccAlpha - 1; p++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't these changes from Anna's PR? do you need her changes duplicated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't need the duplication.
I just fetched and rebased, not sure how it ended up in here... I am not sure how changes happened in between.
I can reset the commit if needed.
#== == == == == == == == == == == == == == == == == == == == == == == == == == \ | ||
== == == == == == == == == == == == # | ||
#Copyright(c) 2024 NVIDIA Corporation &Affiliates.# | ||
#All rights reserved.# | ||
# # | ||
#This source code and the accompanying materials are made available under # | ||
#the terms of the Apache License 2.0 which accompanies this distribution.# | ||
#== == == == == == == == == == == == == == == == == == == == == == == == == == \ | ||
== == == == == == == == == == == == # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the formatter went wild on this one. Did you accidentally run clang-format on this file?
UCCSD operator pool correction and integration in adapt simulator (Issue #28)
This PR fixes #28 .
Problem
Adapt VQE did not provide correct results. The problem could be the creation of the UCCSD operators pool.
Changes
Changes so far have been applied to:
Additional notes
Further testing is needed for making sure that adapt VQE works correctly, especially in terms of molecules.