-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix global phase update in BasisTranslator
Pass (backport #14078)
#14091
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
Conversation
* add missing branch for global phase update in basis_translator * Adding test * reno (cherry picked from commit d67c818) # Conflicts: # crates/accelerate/src/basis/basis_translator/mod.rs # test/python/transpiler/test_basis_translator.py
Cherry-pick of d67c818 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
On hold until #14097 merges |
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.
LGTM
Pull Request Test Coverage Report for Build 14085660796Details
💛 - Coveralls |
Summary
Fixes #14074.
The
basis_translator
pass was not always updating the DAG's global phase as a part ofreplace_node
: this was only done when the target dag's global phase matchedParam::ParameterExpression
. Updating the global phase in the case ofParam::Float
was missing, and this is fixed now.I am somewhat unsure if we also need to handle the
Param::Obj
case. I don't understand in what kind of a circuit we would see this, and I don't believe it's even supported, as perdag_circuit.rs.add_global_phase
, which throws an error in the case ofParam::Obj
. Update: @jakelishman confirmed thatParam::Obj
is never permitted inglobal_phase
.Update: added release notes.
This is an automatic backport of pull request #14078 done by [Mergify](https://mergify.com).