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

Cleans up code and refactors tests for single qubit unitary decompositions #4869

Merged
merged 19 commits into from Nov 23, 2023

Conversation

astralcai
Copy link
Contributor

@astralcai astralcai commented Nov 22, 2023

Context:

As I am implementing the XZX single qubit decomposition (#4862), I noticed some issues with the the code and the tests for single qubit unitary decompositions.

Description of the Change:

Cleans up transforms/decompositions/single_qubit_unitary.py.

  • Removes duplicate code,
  • Fixes doc examples that were untrue
  • Adds support for returning global phase for the rot decomposition.
  • Normalize all rotation angles to the range $[0, 4\pi]$

Refactors tests/transforms/test_decompositions.py.

  • Generalizes logic for obtaining decompositions and asserting their correctness such that all types of decompositions share the same assertion logic. Removes duplicate code.
  • Adds testing for when return_global_phase=False as well as when return_global_phase=True
  • Update test cases to use analytical instead of numerical values when possible.

Benefits:

Cleaner code.

Related GitHub Issues:

#4868
#4862

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1e6c11d) 99.65% compared to head (83bea0f) 99.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4869      +/-   ##
==========================================
- Coverage   99.65%   99.64%   -0.02%     
==========================================
  Files         383      383              
  Lines       34571    34314     -257     
==========================================
- Hits        34451    34191     -260     
- Misses        120      123       +3     

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

@astralcai astralcai marked this pull request as ready for review November 22, 2023 14:18
@rmoyard
Copy link
Contributor

rmoyard commented Nov 22, 2023

Hi @astralcai thanks a lot for this follow up PR. I am not sure it is possible to change the base of this PR because you are external, but that would be nice. Currently it is hard to distinguish your two PRs. If it is not possible, I will wait for the first one to be merged before reviewing.

All rotation angles will be normalized to the range [-2*pi, 2*pi]
for consistency. Additionally, test cases are updated to use
analytical results for readability.
@astralcai
Copy link
Contributor Author

astralcai commented Nov 22, 2023

Hi @astralcai thanks a lot for this follow up PR. I am not sure it is possible to change the base of this PR because you are external, but that would be nice. Currently it is hard to distinguish your two PRs. If it is not possible, I will wait for the first one to be merged before reviewing.

@rmoyard Hi, I have made another update to this PR, now normalizing all rotation angles to $[-2\pi, 2\pi]$ for consistency and easy of use (previously different types decompositions deal with this differently). I have also updated all test cases for single-qubit unitary decompositions to use analytical values as expected parameters when possible.

@rmoyard
Copy link
Contributor

rmoyard commented Nov 22, 2023

Hi @astralcai ! Once you solve the conflicts I will review this PR. One thing about angles for rotation, our canonical range is [0, 4pi], same as Numpy. So it would be best to remove your angle change from this PR. Thanks 👍

@astralcai
Copy link
Contributor Author

Hi @astralcai ! Once you solve the conflicts I will review this PR. One thing about angles for rotation, our canonical range is [0, 4pi], same as Numpy. So it would be best to remove your angle change from this PR. Thanks 👍

@rmoyard In this case, I will update the code for the rot decomposition to normalize its rotation angles to the [0, 4pi] range instead.

Can I ask why this range is chosen?

@astralcai
Copy link
Contributor Author

Hi @astralcai ! Once you solve the conflicts I will review this PR. One thing about angles for rotation, our canonical range is [0, 4pi], same as Numpy. So it would be best to remove your angle change from this PR. Thanks 👍

@rmoyard In this case, I will update the code for the rot decomposition to normalize its rotation angles to the [0, 4pi] range instead.

Can I ask why this range is chosen?

@rmoyard I have updated the PR to remove the angle change. It is ready for review now.

@rmoyard
Copy link
Contributor

rmoyard commented Nov 22, 2023

Thank you I will take a look between today and tomorrow. From what I remember, we wanted to follow the convention by numpy.mod and % where the rest is not negative. We thought it was a better software friendly convention compared to the other one which comes more from physics.

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the tests and the changes to rot. It looks great, I have left some comments. Happy to approve after that 💯

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
tests/ops/qubit/test_matrix_ops.py Show resolved Hide resolved
tests/transforms/test_unitary_to_rot.py Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lillian542 lillian542 left a comment

Choose a reason for hiding this comment

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

I don't have anything to add to @rmoyard's feedback. Thanks @astralcai, this looks great! 🚀

@rmoyard rmoyard self-requested a review November 23, 2023 17:38
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks @astralcai, feel free to merge the PR 💯

@astralcai
Copy link
Contributor Author

Thanks @astralcai, feel free to merge the PR 💯

Hi, thank you. I don't think I have permission to merge this PR.

@rmoyard rmoyard merged commit b43e809 into PennyLaneAI:master Nov 23, 2023
35 checks passed
@astralcai astralcai deleted the decomps-partial-ref branch November 28, 2023 21:22
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.

None yet

3 participants