-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add the state_vector
, measurement
class and simulate
method for the LightningGPU with the new device API
#892
Conversation
Hello. You may have forgotten to update the changelog!
|
state_vector
, measurement
class and simulate
method for the LightningGPU with the new device API
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gpuNewAPI_backend #892 +/- ##
======================================================
+ Coverage 74.66% 93.55% +18.89%
======================================================
Files 27 76 +49
Lines 1954 8801 +6847
======================================================
+ Hits 1459 8234 +6775
- Misses 495 567 +72 ☔ View full report in Codecov by Sentry. |
diagonalizing_gates = measurementprocess.diagonalizing_gates() | ||
if diagonalizing_gates: | ||
self._qubit_state.apply_operations(diagonalizing_gates) | ||
results = self._measurement_lightning.probs(measurementprocess.wires.tolist()) | ||
if diagonalizing_gates: | ||
self._qubit_state.apply_operations( | ||
[qml.adjoint(g, lazy=False) for g in reversed(diagonalizing_gates)] | ||
) | ||
return results |
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.
LGPU has different behaviour in probs
from LQ and LK. LGPU needs to perform an extra conversion of results due to cuQuantum returns as col-major orderings, so performing transpose on data for bit-index shuffle is needed.
pennylane_lightning/core/src/simulators/lightning_gpu/measurements/MeasurementsGPU.hpp
Show resolved
Hide resolved
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
""" |
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.
A new file and class will be used to keep the device implementation similar between LQ, LK, and LGPU. Also, in case of future implementation of MPI resources in other devices.
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.
Thanks @LuisAlfredoNu . A few more questions.
num_wires, | ||
dtype=np.complex128, | ||
device_name="lightning.gpu", | ||
mpi_handler=None, |
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.
Do we want to pass mpi_handler
around?
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.
Yes because I tried to keep the MPI parts inside the LGPU class but at some point I had circular import problems. That is why I created an independent class. Also, It will be necessary to pass mpi_handler
to get the functionality of the MPI methods.
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.
Please let us know if these coverage issues are some fluke.
# TODO | ||
# state_data = allocate_aligned_array(state.size, np.dtype(self.dtype), True) | ||
# state.getState(state_data) | ||
# state = state_data |
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.
When do we plan to do that?
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.
With luck, after completing the migration of LGPU. However, do you think that this will be necessary to develop? 🤔
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.
Looks good to me, thanks @LuisAlfredoNu . I left some comments, feel free to address them as you see fit. About the .
s in docstrings, we could do a full sync across all modules in another PR.
Returns: | ||
Probabilities of the supplied observable or wires | ||
""" | ||
diagonalizing_gates = measurementprocess.diagonalizing_gates() |
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.
Seems we're missing a test 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 do not understand why CodeCov never catch the data from LKokkos 😕
Co-authored-by: Vincent Michaud-Rioux <[email protected]>
The coverage issues are related to the MPI part which is under development and has not been tested on the current PR. Although, this does not affect the code for a single device. For LK, I do know why CodeCov never catch the coverage of this device. 😕 |
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.
Nothing more to add. Thank you for that!
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.
Thanks @LuisAlfredoNu! Happy to approve the PR after addressing the design-related concerns.
@pytest.mark.skipif( | ||
device_name == "lightning.gpu", | ||
reason="lightning.gpu does not support out of order prob.", | ||
) |
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.
@multiphaseCFD Is there any 3rd-party blocker to add the out-of-order prob support?
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.
cuquantum
libs does not support out of order probs.
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 this case, I'm wondering if we could: order the wires, calculate the probs, and transpose back to the unordered set of wires. @multiphaseCFD, did we try it already? Do you see any blockers for it? I'm just thinking about it, nothing to do now, of course.
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.
Thanks @LuisAlfredoNu! Just a few minor suggestions before merging the PR 🥳
pennylane_lightning/core/src/simulators/lightning_gpu/StateVectorCudaManaged.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_gpu/StateVectorCudaManaged.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Ali Asadi <[email protected]>
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.
LGTM! Thanks @LuisAlfredoNu !
num_local_wires = len(probs_results).bit_length() - 1 if len(probs_results) > 0 else 0 | ||
return probs_results.reshape([2] * num_local_wires).transpose().reshape(-1) | ||
|
||
return probs_results |
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.
Is this line missed in the unit tests or a false positive?
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.
Yes, you are right, this line is not covered. I propose to let it and try to catch the coverage in the following PR 😄
@pytest.mark.skipif( | ||
device_name == "lightning.gpu", | ||
reason="lightning.gpu does not support out of order prob.", | ||
) |
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.
cuquantum
libs does not support out of order probs.
Before submitting
Please complete the following checklist when submitting a PR:
All new features must include a unit test.
If you've fixed a bug or added code that should be tested, add a test to the
tests
directory!All new functions and code must be clearly commented and documented.
If you do make documentation changes, make sure that the docs build and
render correctly by running
make docs
.Ensure that the test suite passes, by running
make test
.Add a new entry to the
.github/CHANGELOG.md
file, summarizing thechange, and including a link back to the PR.
Ensure that code is properly formatted by running
make format
.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context:
Migrate LightningGPU to the new device API
Description of the Change:
Create the
state_vector
, andmeasurement
class for the new device API to achieve thesimulate
methodBenefits:
Integration of LGPU with the new device API
Possible Drawbacks:
Related GitHub Issues:
Freezzed PR⚠️ ❄️
To make a smooth integration of LightningGPU with the new device API, we set the branch
gpuNewAPI_backend
as the base branch target for future developments related to this big task.The branch
gpuNewAPI_backend
has the mock of all classes and methods necessary for the new API. Also, several tests were disabled withHowever, these tests will unblocked as the implementation progresses.
After all the developments for integrating LightningGPU with the new API have been completed then the PR will be open to merge to
master
[sc-70932]