Skip to content

Fix global phase tracking around DAGCircuit.substitute_node. (bp #5618)#5673

Merged
mergify[bot] merged 6 commits into
stable/0.16from
mergify/bp/stable/0.16/pr-5618
Jan 25, 2021
Merged

Fix global phase tracking around DAGCircuit.substitute_node. (bp #5618)#5673
mergify[bot] merged 6 commits into
stable/0.16from
mergify/bp/stable/0.16/pr-5618

Conversation

@mergify
Copy link
Copy Markdown
Contributor

@mergify mergify Bot commented Jan 21, 2021

This is an automatic backport of pull request #5618 done by Mergify.

Cherry-pick of 89cd41e has failed:

On branch mergify/bp/stable/0.16/pr-5618
Your branch is up to date with 'origin/stable/0.16'.

You are currently cherry-picking commit 89cd41e57.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   qiskit/circuit/quantumcircuit.py
	modified:   qiskit/dagcircuit/dagcircuit.py
	modified:   qiskit/transpiler/passes/basis/basis_translator.py
	modified:   qiskit/transpiler/passes/basis/unroller.py
	new file:   releasenotes/notes/dagcircuit-fix-substitute-node-global-phase-usage-ee05476a7d0b24c6.yaml
	modified:   test/python/circuit/library/test_grover_operator.py
	modified:   test/python/circuit/test_circuit_qasm.py
	modified:   test/python/transpiler/test_unroller.py

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   test/python/compiler/test_transpiler.py
	both modified:   test/python/transpiler/test_basis_translator.py

To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.io/

@mergify mergify Bot requested a review from a team as a code owner January 21, 2021 20:25
@mergify mergify Bot added the conflicts used by mergify when there are conflicts in a port label Jan 21, 2021
@mtreinish mtreinish added the Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. label Jan 21, 2021
* DAGCircuit.__eq__ to check .global_phase and .calibrations.

* Fix BasisTranslator and Unroller global phase for substitute_node.

* Update releasenotes/notes/dagcircuit-fix-substitute-node-global-phase-usage-ee05476a7d0b24c6.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update releasenotes/notes/dagcircuit-fix-substitute-node-global-phase-usage-ee05476a7d0b24c6.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 89cd41e)
@kdk kdk force-pushed the mergify/bp/stable/0.16/pr-5618 branch from 20e50c7 to 44092e4 Compare January 21, 2021 22:08
@mtreinish mtreinish added automerge and removed conflicts used by mergify when there are conflicts in a port labels Jan 21, 2021
@mtreinish mtreinish mentioned this pull request Jan 22, 2021
6 tasks
Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Actually, looking at this more closely with fresh eyes I think we should remove the __eq__ changes from a backport because it's potentially a disruptive upgrade for users

Comment thread qiskit/dagcircuit/dagcircuit.py Outdated

with self.subTest('circuits match'):
expected = QuantumCircuit(*grover_op.qregs)
expected = QuantumCircuit(*grover_op.qregs, global_phase=np.pi)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this isn't correct on stable/0.16, the global phase of pi was added to the grover operator in #5517: https://github.com/Qiskit/qiskit-terra/pull/5517/files#diff-5cc16ff691e55c7c0e9d381f48c31157dcc945f3107f4e6ea246b2a2bcce986e we should check with @Cryoris if that needs to be backported too or not

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can include that phase change from 5517, since then the Grover operator fits the definition from the papers 🙂

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok, I'll add that here then

This commit adds a missing phase of pi to the grover operator circuit
library entry. It is a backport from #5517.
In #5618 the dagcircuit __eq__ method was updated to also compare the
global_phase and calibrations property. This was done to ensure we
preserve both through a transpilation and other transforms since when we
assert a circuit is equal to the expect result it will also check phase.
However, this has backwards compatibility implications since comparing 2
QuantumCircuits would previously ignore global phase. This commit
removes this piece from the backport and just leave the phase
corrections as the backport.
@mergify mergify Bot merged commit 1e09c16 into stable/0.16 Jan 25, 2021
@mergify mergify Bot deleted the mergify/bp/stable/0.16/pr-5618 branch January 25, 2021 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants