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

Fix zero-operand gates and instructions #8272

Merged
merged 7 commits into from
Oct 29, 2022

Conversation

jakelishman
Copy link
Member

Summary

A few checks in different places didn't account for zero-operand gates
being possible. In most uses, there never would be anything useful
here, but it can occasionally be useful to create a "global-phase"
instruction, especially if this is later going to have controls applied
to it. This is well-formed within the Qiskit data model; spectator
qubits implicitly undergo the identity, so this corresponds to the
global phase being multiplied by the identity on all qubits.

Details and comments

This came up in #8236. This PR doesn't add the requested gate, it just removes the roadblocks from a user-defined definition - such a gate should already be representable in the Gate.definition form, but due to the bugs fixed here, would fail in weird ways.

There's probably more places where this edge case will slip through and make a mess that I didn't catch here, but this makes everything I could immediately think of work - transpilation at pre-defined levels, gate control, conversion to Operator, etc.

A few checks in different places didn't account for zero-operand gates
being possible.  In most uses, there never would be anything useful
here, but it can occasionally be useful to create a "global-phase"
instruction, especially if this is later going to have controls applied
to it.  This is well-formed within the Qiskit data model; spectator
qubits implicitly undergo the identity, so this corresponds to the
global phase being multiplied by the identity on _all_ qubits.
@jakelishman jakelishman added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jun 29, 2022
@jakelishman jakelishman requested review from a team and ikkoham as code owners June 29, 2022 12:04
@qiskit-bot
Copy link
Collaborator

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 the following people are requested to review this:

@coveralls
Copy link

coveralls commented Jun 29, 2022

Pull Request Test Coverage Report for Build 3350717692

  • 12 of 13 (92.31%) changed or added relevant lines in 4 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 84.451%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/controlledgate.py 7 8 87.5%
Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 91.49%
Totals Coverage Status
Change from base Build 3348990138: 0.04%
Covered Lines: 62173
Relevant Lines: 73620

💛 - Coveralls

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

It feels a little weird to add a controlled global phase this way but if it's required that's fine by me. So this is to add a global phase on the entire circuit right? If we need a relative phase we could also do something like

phase = QuantumCircuit(1, global_phase=phase)
phase.control(...)

qiskit/circuit/controlledgate.py Outdated Show resolved Hide resolved
@jakelishman
Copy link
Member Author

The example you gave is similar to what's being asked for in the linked issue, yeah. It's possible to get around that by other means, but these fixes here are stuff that should work anyway. It's a bit awkward that you need to use a create a full QuantumCircuit to apply a true global phase, and it's awkward that at the moment you somewhat arbitrarily need to apply it to one-or-more qubits, when our rules of "spectator qubits implicitly undergo the identity" should mean that you only need to apply it to zero.

Basically: there's ways to get the desired effect in the linked issue, but this stuff should work anyway, and a very basic "global phase" gate seems like a convenient utility definition we could supply.

@jakelishman jakelishman mentioned this pull request Jun 29, 2022
@jakelishman
Copy link
Member Author

I've fixed the merge conflict and added an extra test for Erick.

@jakelishman jakelishman added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Oct 21, 2022
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but now something like

my_cx = ControlledGate(
    name="cx?", 
    num_qubits=2, 
    params=[],
    num_ctrl_qubits=2, # same as num_qubits!
)

is technically a valid input right? Could we add a test to ensure this breaks as expected?

Also in the class docstring is still says that

            CircuitError: If ``num_ctrl_qubits`` >= ``num_qubits``.

maybe

            CircuitError: If ``num_ctrl_qubits + base_gate.num_qubits`` >= ``num_qubits``.

would be more accurate? 🙂

@jakelishman
Copy link
Member Author

Oh, good idea on the last part - that's the constraint that actually should be implemented. We did need the case of num_qubits == num_ctrl_qubits to be allowed so that something like ControlledGate(base_gate=GlobalPhaseGate(pi)) could succeed, but you're right it should still error out for things like base_gate=XGate(). (Of course, the potential GlobalPhaseGate().control() would actually just return PhaseGate(), but the point stands that the ControlledGate constructor should be able to take it.)

@jakelishman
Copy link
Member Author

Done in af1abf0.

@mergify mergify bot merged commit bb93592 into Qiskit:main Oct 29, 2022
mergify bot pushed a commit that referenced this pull request Oct 29, 2022
* Fix zero-operand gates and instructions

A few checks in different places didn't account for zero-operand gates
being possible.  In most uses, there never would be anything useful
here, but it can occasionally be useful to create a "global-phase"
instruction, especially if this is later going to have controls applied
to it.  This is well-formed within the Qiskit data model; spectator
qubits implicitly undergo the identity, so this corresponds to the
global phase being multiplied by the identity on _all_ qubits.

* Fix controlled-gate qubit-number documentation

* Add ScalarOp test

* Be stricter about allowed num_ctrl_qubits

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit bb93592)
mergify bot added a commit that referenced this pull request Oct 29, 2022
* Fix zero-operand gates and instructions

A few checks in different places didn't account for zero-operand gates
being possible.  In most uses, there never would be anything useful
here, but it can occasionally be useful to create a "global-phase"
instruction, especially if this is later going to have controls applied
to it.  This is well-formed within the Qiskit data model; spectator
qubits implicitly undergo the identity, so this corresponds to the
global phase being multiplied by the identity on _all_ qubits.

* Fix controlled-gate qubit-number documentation

* Add ScalarOp test

* Be stricter about allowed num_ctrl_qubits

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit bb93592)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
@jakelishman jakelishman deleted the fix/converters-zero-operands branch January 16, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants