-
Notifications
You must be signed in to change notification settings - Fork 39
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
Closes #557, in-place implementation of expval for LightningQubit #565
base: master
Are you sure you want to change the base?
Conversation
* Add operations functors in ExpValFunctorsLQubit.hpp * Add method to call functors in Measurement class * Add enum class for functors * Add mapping for operation names to functors
* Fix types and const to pass tidy, cpp, format, coverage tests
* Added Hadamard and Identity tests to 'Expval - NamedObs'
@Alex-Preciado Please see here for the benchmark script and its artifacts. |
Thank you so much for preparing this PR, @sbohloul ! The team will have a look next week, and we will come back to you with feedback. Let me know if you have any other questions in the meantime. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #565 +/- ##
==========================================
- Coverage 98.70% 96.95% -1.75%
==========================================
Files 168 146 -22
Lines 22763 18113 -4650
==========================================
- Hits 22468 17562 -4906
- Misses 295 551 +256 ☔ View full report in Codecov by Sentry. |
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.
Hi @sbohloul, thank you for your contribution.
The Codecov report shows that some areas of your code are not been touched by the test suite. Could you please expand tests, so you can improve code coverage?
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 for the submission @sbohloul
As suggested by @AmintorDusko it would be great to see some additional test coverage as reported by the CI.
Also, I have some questions based on the implementations:
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/MeasurementsLQubit.hpp
Outdated
Show resolved
Hide resolved
* Fix identity operator * Move tests to 'Test_MeasurementsLQubit.cpp' and add Identity and Hadamard tests
* Add docstrings for functors * Return expval by value instead of passing by reference * Make fuctors data members private * Use initializer list for functors constructors * Make isqrt2 * Make expval_funcs_ const and initialize with list initializer
@AmintorDusko Thank you for your feedback. I expanded the coverage as suggested. |
@mlxd Thank you for your feedback and questions. I applied several of your comments. Please find my answers in correspoding section. |
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.
Thank you @sbohloul, for the improvement in coverage. I have a few more questions/suggestions.
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/MeasurementsLQubit.hpp
Outdated
Show resolved
Hide resolved
...lightning/core/src/simulators/lightning_qubit/measurements/tests/Test_MeasurementsLQubit.cpp
Show resolved
Hide resolved
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 a lot @sbohloul
Some final comments from me on this. Great work on the implementation.
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Outdated
Show resolved
Hide resolved
pennylane_lightning/core/src/simulators/lightning_qubit/measurements/ExpValFunctorsLQubit.hpp
Show resolved
Hide resolved
* Change functors from struct to class
@Alex-Preciado I was wondering if there are any other requirements to be fulfilled regarding this PR. |
Thanks for the submission @sbohloul :) Happy to say we can come to this PR at a later date --- nothing left to do here for now. The team will reach out to you shortly about the next steps. |
@mlxd @AmintorDusko Thank you again for your helpful comments to navigate this PR. I would be happy to finalize the work here since we already spent some time on it. Please let me know what are your toughts about this. |
case ExpValFunc::Identity: | ||
return applyExpValNamedFunctor<getExpectationValueIdentityFunctor, | ||
0>(wires); |
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.
Hi @sbohloul , nice work there. If you have time, I'd like us to update and merge this PR. We should first merge master
in this branch, update the CHANGELOG and fix conflicts accordingly. Next, I would like us to change the implementation to avoid the use of functors. This is too much boilerplate for host-only code (sometimes required for device-generic code as in Kokkos however). Could you have a look at the LM kernels's applyNC1
? Could we write a lambda-templated method that accumulates the expval of a generic gate? We could then simply write something like
switch (expval_funcs_.at(operation)) {
[...]
case ExpValFunc::PauliX:
auto core_function = [](std::complex<PrecisionT> *arr,
const std::size_t i0, const std::size_t i1) {
std::swap(arr[i0], arr[i1]);
};
[...]
}
applyExpVal1<PrecisionT, decltype(core_function)>(wires, core_function);
Context:
This PR modifies the expval method for named operations using an in-place, OpenMP parallel, implementation to avoid creating an extra intemediate statevector.
Description of the Change:
The implementation closely follows that of MeasurementsKokkos.hpp. It adds the corresponding functors in
ExpValFunctorsLQubit.hpp
and modifiesMeasurementsLQubit.hpp
accordingly. It uses the existing tests of named operators inTest_MeasurementsLQubit.cpp
to validate the changes.Benefits:
PauliX
,PauliY
,PauliZ
,Hadamard
, andIdentity
operators. See performance benchmark for some comparisions.Possible Drawbacks:
Related GitHub Issues:
Closes #557
[sc-65192]