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.

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 GlobalPhase to all devices #579

Merged
merged 73 commits into from
Feb 15, 2024
Merged

Add GlobalPhase to all devices #579

merged 73 commits into from
Feb 15, 2024

Conversation

vincentmr
Copy link
Contributor

@vincentmr vincentmr commented Dec 13, 2023

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 the
    change, 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:
Prior to #516 , the Lightning devices could not properly run Grover's algorithm (among others) because GlobalPhase and C(GlobalPhase) are not supported. This would also be true of any circuit which ends up with GlobalPhase or C(GlobalPhase) after decomposition. In addition, L-GPU's custom gate cache was not passed a parameter to distinguish between gates. This meant, for example, that MultiControlledX([0, 1, 2], "111") and MultiControlledX([0, 2], "00") were applied as the same operation. This likely affected any circuit with more than one version of

"QubitUnitary",
"ControlledQubitUnitary",
"MultiControlledX",
"DiagonalQubitUnitary",
"PSWAP",
"OrbitalRotation",

Description of the Change:
Add GlobalPhase and C(GlobalPhase) support in L-GPU and L-Kokkos. For the sake of time, C(GlobalPhase) is implemented by generating the diagonal entries with global_phase_diagonal at the Python layer, and then passing it through the gate matrix argument down to the C++ layer and adding a special branch for it in applyOperation. To fix the gate cache, we pass the gate hash as a parameter when applying any of the above gates to make sure the gate cache adds a new matrix.

Benefits:
Bug fix, can now run Grover with L-GPU and L-Kokkos.

Possible Drawbacks:
Higher memory demand in the gate cache.

Related GitHub Issues:

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (2143239) 98.96% compared to head (5c233ef) 98.47%.

Files Patch % Lines
.../simulators/lightning_kokkos/StateVectorKokkos.hpp 8.00% 23 Missing ⚠️
...imulators/lightning_gpu/StateVectorCudaManaged.hpp 14.28% 18 Missing ⚠️
...ators/lightning_kokkos/gates/GateFunctorsParam.hpp 0.00% 7 Missing ⚠️
...tning_qubit/gates/tests/Test_OpToMemberFuncPtr.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #579      +/-   ##
==========================================
- Coverage   98.96%   98.47%   -0.49%     
==========================================
  Files         169      169              
  Lines       24308    24387      +79     
==========================================
- Hits        24056    24015      -41     
- Misses        252      372     +120     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vincentmr
Copy link
Contributor Author

Grover algorithm returns wrong results with [sc-51671]

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Thanks @vincentmr
I think we can come back and discuss this in a few weeks

@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (1e07379) 98.49% compared to head (95f4f2a) 98.72%.

Files Patch % Lines
...lane_lightning/core/src/utils/TestHelpersWires.hpp 66.66% 4 Missing ⚠️
...tning_qubit/gates/tests/Test_OpToMemberFuncPtr.cpp 0.00% 2 Missing ⚠️
...gates/tests/Test_GateImplementations_Generator.cpp 97.61% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #579      +/-   ##
==========================================
+ Coverage   98.49%   98.72%   +0.22%     
==========================================
  Files         169      169              
  Lines       24717    24557     -160     
==========================================
- Hits        24345    24243     -102     
+ Misses        372      314      -58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Hi @vincentmr, I left a few comments.

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Thanks @vincentmr
Just a few final questions, and will be happy to approve once they are commented on.

@vincentmr vincentmr merged commit 102d848 into master Feb 15, 2024
84 of 85 checks passed
@vincentmr vincentmr deleted the feature/lq_globalphase branch February 15, 2024 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants