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

Decomposition using GlobalPhase has real phase #4653

Merged
merged 11 commits into from
Oct 13, 2023
Merged

Conversation

lillian542
Copy link
Contributor

@lillian542 lillian542 commented Oct 5, 2023

Context:
When we added the GlobalPhase and updated the decompositions for QubitUnitary to use it, we left the datatype for the phase as complex, even though the results are real

Description of the Change:
There is one line where the angles, though per definition real, need to have a complex dtype to avoid making PyTorch angry. They are now cast to real only there in that line, rather than carrying that through everywhere else.

Benefits:
Its cleaner

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@lillian542 lillian542 marked this pull request as ready for review October 5, 2023 21:25
@lillian542 lillian542 changed the title Decomposition into GlobalPhase is real Decomposition using GlobalPhase has real phase Oct 5, 2023
@lillian542
Copy link
Contributor Author

[sc-44772]

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2f1ceb3) 99.65% compared to head (5f183f0) 99.65%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4653   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         376      376           
  Lines       33702    33702           
=======================================
  Hits        33585    33585           
  Misses        117      117           
Files Coverage Δ
.../transforms/decompositions/single_qubit_unitary.py 100.00% <100.00%> (ø)

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

@lillian542 lillian542 requested review from a team and timmysilv October 6, 2023 13:54
@trbromley
Copy link
Contributor

Sorry if this is WIP, but please add to the changelog.

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

👍 Nice and clean :)

@timmysilv
Copy link
Contributor

This matches #4639 in that they're both attempting to identify real values with dtype=complex. Are they different problems? If everyone needs to set their own thresholds and their own decisions for when to cast, then nvm. But if not, I'd like to standardize how PennyLane deems "approximately real" values to be truly real, and casts them (likely in qml.math). Lmk thoughts.

Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

🎉

@lillian542 lillian542 enabled auto-merge (squash) October 13, 2023 18:01
@lillian542 lillian542 merged commit 82bab31 into master Oct 13, 2023
39 checks passed
@lillian542 lillian542 deleted the real-global-phase branch October 13, 2023 19:04
@albi3ro
Copy link
Contributor

albi3ro commented Nov 10, 2023

Fixes #4565

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

4 participants