-
Notifications
You must be signed in to change notification settings - Fork 150
handle complex phases in .to_adjoint and .conjugate #394
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
base: master
Are you sure you want to change the base?
handle complex phases in .to_adjoint and .conjugate #394
Conversation
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.
Pull request overview
This PR fixes issue #380 by implementing proper handling of complex phases in the .to_adjoint() and .conjugate() methods across the PyZX codebase. Previously, these methods simply negated phases, which is incorrect for complex-valued phases that can arise in symbolic computations.
Changes:
- Added
Poly.conjugate()method to properly conjugate complex coefficients in polynomials - Updated
Scalar.copy()to handle complex phases using(-phase).conjugate()transformation - Modified
Gate.to_adjoint()to apply the same transformation for complex Poly phases - Added comprehensive test coverage for all three components
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pyzx/symbolic.py | Adds conjugate() method to Poly class that conjugates complex coefficients while preserving real coefficients |
| pyzx/graph/scalar.py | Updates copy() method with helper function to properly conjugate complex phases in all scalar fields |
| pyzx/circuit/gates.py | Modifies to_adjoint() to detect Poly phases and apply (-phase).conjugate() transformation |
| tests/test_symbolic_parsing.py | Adds test class with coverage for real, complex, and pure imaginary polynomial coefficients |
| tests/test_scalar.py | Adds tests for scalar conjugation with complex phases in phase, phasenodes, and sum_of_phases |
| tests/test_circuit.py | Adds tests for gate adjoint operations with real, symbolic, and complex phases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pyzx/circuit/gates.py
Outdated
| g.phase = -g.phase | ||
| phase = g.phase | ||
| if isinstance(phase, Poly): | ||
| g.phase = (-phase).conjugate() |
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.
Does this make sense? It looks like you are conjugating twice
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 don't understand. To compute the adjoint of e^{i π phase}, we need e^{-i π phase*} where phase* is the complex conjugate of phase. So (-phase).conjugate() is the correct value, isn't it?
Is the brackets that's the problem? Since negation and conjugation commute, I can remove them.
pyzx/graph/scalar.py
Outdated
| """ | ||
| def conjugate_phase(phase: FractionLike) -> FractionLike: | ||
| if isinstance(phase, Poly): | ||
| return (-phase).conjugate() |
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.
Same here, it looks like you are conjugating twice
jvdwetering
left a comment
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'm a little bit confused about certain lines where it looks like you are conjugating the value twice. Could you take a look?
7fd77a2 to
210a4d2
Compare
|
I think I encountered a bug while trying to write a test for this. In these lines from In |
|
Ah, that was noticed in #377 already, but for |
86340ff to
bcda7f9
Compare
bcda7f9 to
21b6f04
Compare
Fixes #380.