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

DAGCircuit: Add get_causal_cone method #10325

Merged
merged 26 commits into from
Jul 18, 2023

Conversation

raynelfss
Copy link
Contributor

Summary

The causal node of a qubit is the set of all the qubits that affect the resulting output of the aforementioned qubit. This PR aims to add the get_causal_node method to DAGCircuit to obtain the causal node of a qubit in a circuit.

A couple of helper methods have also been added:

  • get_qubit_input_node: to get the input node of a certain qubit in the circuit.
  • get_qubit_output_node: to get the output node of a certain qubit in the circuit.

Details and comments

  • The implementation of these methods may not be the most optimal one, so it is open to suggestions from the community.

@raynelfss raynelfss requested a review from a team as a code owner June 23, 2023 04:37
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 23, 2023
@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:

  • @Qiskit/terra-core

- Type checking was causing strange behavior during linting.
@coveralls
Copy link

coveralls commented Jun 23, 2023

Pull Request Test Coverage Report for Build 5562839866

  • 16 of 17 (94.12%) changed or added relevant lines in 1 file are covered.
  • 20 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.01%) to 86.053%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/dagcircuit/dagcircuit.py 16 17 94.12%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 1 90.33%
qiskit/extensions/quantum_initializer/squ.py 2 80.0%
crates/qasm2/src/lex.rs 5 91.14%
crates/qasm2/src/parse.rs 12 97.11%
Totals Coverage Status
Change from base Build 5559766630: -0.01%
Covered Lines: 72273
Relevant Lines: 83987

💛 - Coveralls

@mtreinish mtreinish added Changelog: New Feature Include in the "Added" section of the changelog Intern PR PR submitted by IBM Quantum interns labels Jun 23, 2023
@CLAassistant
Copy link

CLAassistant commented Jun 26, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ raynelfss
✅ danielleodigie
❌ Raynel Sanchez


Raynel Sanchez seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jakelishman jakelishman removed the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 30, 2023
@mtreinish mtreinish added this to the 0.25.0 milestone Jul 10, 2023
Copy link
Contributor

@danielleodigie danielleodigie left a comment

Choose a reason for hiding this comment

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

I think this looks good to me!

Copy link
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.

This is a great start, thanks for getting it going. I left a few inline comments on some small cleanups you can make to the interface to make the implementation a bit simpler and also more consistent with the rest of the DAGCircuit API.

qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
Copy link
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.

Thanks for the fast updates, just a couple of small comments inline, mostly about doc formatting.

qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
Copy link
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.

I think this is basically ready now, just a couple of small inline comments, but after that I think it's ready to merge

Returns:
Set[Qubit]: The set of qubits whose interactions affect ``qubit``.
"""
# Retrieve the output node and the qubit
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Retrieve the output node and the qubit
# Retrieve the output node from the qubit

# Check if there are any qubits in common and that the operation is not a barrier.
if (
len(qubit_set.intersection(qubits_to_check)) > 0
and not node_to_check.op.name == "barrier"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
and not node_to_check.op.name == "barrier"
and node_to_check.op.name != "barrier"

Comment on lines 1924 to 1927
if (
len(qubit_set.intersection(qubits_to_check)) > 0
and not node_to_check.op.name == "barrier"
):
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to add a check on: not getattr(node_to_check.op, "_directive") which is an expansion of the barrier check so it'll exclude other directives too, like snapshot from aer, etc.

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.

I think your code will already handle this correctly, but it'd be good to add a test that this function will include qubits in the causal cone that interact with the target qubit only via a classical condition.

For example,

qc = QuantumCircuit(2, 1)
qc.x(0)
qc.measure(0, 0)
qc.x(1).c_if(0, True)

In that circuit, qubit 0 is in the causal cone of qubit 1, but it never appears in a 2q quantum operation. I'm fairly sure your code already handles this right, but it'd be good to have the extra test.

@raynelfss
Copy link
Contributor Author

I think your code will already handle this correctly, but it'd be good to add a test that this function will include qubits in the causal cone that interact with the target qubit only via a classical condition.

Well, I just tried this with the code and it doesn't seem to handle it well. Since the controlled operation never happens directly in a multiqubit operation and the condition is happening based on a classical condition, it will ignore it as the function only considers qubits and not classical bits.

This is an interesting test case though, and raises a great question of how direct do interactions have to be for it to be considered as part of the causal cone. In this case, since the output of qubit 0 affects the output of qubit 1, it should be part of its causal cone.

I will try to work that into the code, thank you for bringing it up @jakelishman.

@jakelishman
Copy link
Member

Thinking about it, it actually gets very hairy very quickly, because with clbits there's a distinction between "writes" and "reads", which the DAGCircuit doesn't represent right now. For example, if I change my example to:

qc = QuantumCircuit(3, 1)  # Note the extra qubit
qc.x(0)
qc.measure(0, 0)
qc.x(1).c_if(0, True)
qc.x(2).c_if(0, True)

Now the qc.x(1) operation will be a DAGCircuit predecessor of the x(2) operation, but qubit 1 isn't in the causal cone of qubit 2, because it doesn't write to the wire that orders them.

I'm not sure that it's going to be reasonable to handle this at the moment; the DAGCircuit just doesn't give us the information we need right now to determine it. Matt might have an opinion on what the best API choice for this is.

@mtreinish
Copy link
Member

TBH, I'm thinking it might be better to call this quantum_causal_cone (I feel like this is sounding more and more scifi) and make this explicitly for quantum data dependency only and document it as such. At least then we're punting the question of classical data dependency for now, because at the least the primary use cases for this are only about the quantum side. The question for me then is whether we do the normal dagcircuit thing of treating a .condition as breaking the data dependency (i.e. collect_runs() which terminates the run on a controlled gate) explicitly. Or just documenting it as not being accounted for in the output and not trying to handle it explicitly. In general I think just making explicitly only for quantum data dependencies and documenting it clearly will be enough.

the causal cone of a qubit can be useful when debugging faulty circuits, as it can
help identify which wire(s) may be causing the problem.

This method does not consider classical to quantum influences through conditionals or
Copy link
Member

Choose a reason for hiding this comment

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

I'm finding this a bit hard to follow, maybe reword this a bit to something like?:

Suggested change
This method does not consider classical to quantum influences through conditionals or
This method does not consider any classical data dependency in the ``DAGCircuit``, classical
bit wires are ignored for the purposes of building the causal cone.

qubits_to_check = qubits_to_check.union(qubit_set)
# For each predecessor of the current node, filter input/output nodes,
# also make sure it has at least one qubit in common. Then append.
for node in self.predecessors(node_to_check):
Copy link
Member

Choose a reason for hiding this comment

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

If we want to hold to the statement of ignoring the classical wires should we change this to?:

Suggested change
for node in self.predecessors(node_to_check):
for node in self.quantum_predecessors(node_to_check):

(and above as well).

Copy link
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.

LGTM now, thanks for all the quick updates.

# Add the qubit to the causal cone.
qubits_to_check = {qubit}
# Add predecessors of output node to the queue.
queue = deque(self.predecessors(output_node))
Copy link
Member

Choose a reason for hiding this comment

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

This could be quantum predecessors too, but if the input is a Qubit then there is no way that output_node could have a classical wire into it.

@mtreinish mtreinish enabled auto-merge July 18, 2023 19:39
@mtreinish mtreinish added this pull request to the merge queue Jul 18, 2023
Merged via the queue into Qiskit:main with commit 90c2fe6 Jul 18, 2023
13 checks passed
@raynelfss raynelfss deleted the dagcircuit/causal-cone branch July 21, 2023 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Intern PR PR submitted by IBM Quantum interns
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants