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

enable inverse of classically conditioned gate #6829

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ewinston
Copy link
Contributor

@ewinston ewinston commented Jul 29, 2021

Summary

This PR enables the preservation of the classical condition for the inverse of a classically conditioned gate.
Also added is the ability to initialize the classical condition during instruction instantiation which should help with progress toward issue #2439.

Fixes #6810

Details and comments

@ewinston ewinston requested a review from a team as a code owner July 29, 2021 07:55
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me - a couple of minor comments on documentation only. It's difficult to check completeness in a diff (in the sense that it's difficult to see if condition has been missed anywhere), but it looks like it's gone through all the important files.

qiskit/circuit/instruction.py Outdated Show resolved Hide resolved
qiskit/circuit/controlledgate.py Outdated Show resolved Hide resolved
qiskit/circuit/controlledgate.py Outdated Show resolved Hide resolved
num_qubits: int,
params: List,
label: Optional[str] = None,
condition: Optional[Tuple] = None,
Copy link
Member

Choose a reason for hiding this comment

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

The new docstring for "condition" needs to be here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

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

As a philosophical question, is it necessary to add a condition kwarg to all these gates? Wouldn't it be simpler to set the condition at the Instruction parent level? For example, in x.py,

def inverse(self):
    x_inv = XGate()
    x_inv.condition = self.condition
    return x_inv

This way you're taking care of the issue at the inverse() level and leaving the gate def simpler.

@jakelishman
Copy link
Member

To me, setting parameters after construction is a slightly strange idiom, because you're basically pushing responsibility for initialising an object onto the user, and drawing an arbitrary line between what's passed to the initialiser and what's not (there's also 'label' in the initialiser). There's also a usability concern with this sort of separation: you can't create the object and use it inside another function call any more, because initialisation is two statements. That can be quite an annoyance for people working interactively.

Maybe it's me coming from a slightly more immutable-by-default background, though - I've never done large-scale pure OO.

@enavarro51
Copy link
Contributor

Currently in qiskit, the condition is set at the Instruction parent level and stored there, so individual gates do not have condition attributes. If philosophically you want to change that to add condition kwargs to gates, by your reasoning, it should be done to every gate, and probably not in this PR which is only focused on gates that have their own inverse() method.

@jakelishman
Copy link
Member

jakelishman commented Jul 29, 2021

Gate inherits from Instruction, so I'm not quite sure what you mean by "gates don't have condition attributes". That said, I thought this PR did add the condition kwarg to the constructors of all gates in the library, though I now see that it doesn't. It should be all or nothing, and Instruction should gain it as well if that's the we way things want to go. I'd not be opposed to that - by my arguments above, I think pretty much everything that mutates the object should be able to be done in one statement. This means I don't mind method chaining with return self, though, so XGate().c_if(...) is also ok, just slightly sharp-edged if some mutations can't be set in the initialiser, and some can - label can be mutated, and set, for example.

I still don't like the idea of promoting manually assigning to a parameter, because as it currently stands that elides all error-checking, so it's not what we would want users to do. If we don't want to add to the initialiser, then it might be nice if the inverse functions looked like

def inverse(self):
    return XGate().c_if(*self.condition)

and also the Instruction.condition should became a property whose setter does the error checking.

Co-authored-by: Jake Lishman <jake@binhbar.com>
@enavarro51
Copy link
Contributor

Right. Definitely should be all or nothing, but is this the place to do it? Your def inverse(): idea should work since c_if returns self, but c_if is rarely used in the internal qiskit code outside of testing. Not sure if that's an issue.

@ewinston
Copy link
Contributor Author

ewinston commented Jul 29, 2021

The c_if implementation has occupied a special place in our interface and is the main reason I believe why the gate methods of QuantumCircuit return InstructionSet objects instead of QuantumCircuit (or None). This point has been mentioned in issue #2439 as something to move away from.

I agree with @jakelishman about making it possible to set all properties of the instruction through its __init__ method. It also opens the possibility of creating circuits like circ.h(0).cx(0, 1) which might be convenient.

Also, it was the intention to add this to all gates of the library. I'll add those. Please let me know what I miss.

@jakelishman
Copy link
Member

Instruction.c_if is the only guaranteed-safe way to add a condition to an instruction, and the only allowed user-facing method. It's no accident that c_if returns self, because it's the principle method used to construct circuits with classical control, and all quantum circuit methods that create instructions return the instruction. There shouldn't be (but probably is) user code which directly sets Instruction.condition. This could be fixed by making Instruction.condition a safety-checked property, like I mentioned before.

@ewinston
Copy link
Contributor Author

@jakelishman I'm adding condition as a property as you suggested.

@jakelishman
Copy link
Member

ah, a github-comment race condition! I hadn't seen your message beforehand!

@ewinston ewinston closed this Apr 8, 2022
@ewinston ewinston deleted the inverse_c_if branch April 8, 2022 14:44
@ewinston ewinston restored the inverse_c_if branch June 14, 2022 16:06
@ewinston ewinston reopened this Jul 13, 2022
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.

inverse of conditional gate is not conditional
3 participants