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

Add Expr support in DAGCircuit.substitute_node #10382

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

jakelishman
Copy link
Member

Summary

As part of this minor refactor, this updates the logic to no longer silently override conditions on the replacement op. The method gains a propagate_condition argument analogous to the same argument in substitute_node_with_dag, which can be set False to specify that the caller is aware that the new operation should implement the same conditional logic.

Details and comments

Close #10380. Depends on #10377. This has a partial changelog of itself, but the rest is coming in #10331.

@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jul 4, 2023
@jakelishman jakelishman added this to the 0.25.0 milestone Jul 4, 2023
@jakelishman jakelishman requested a review from a team as a code owner July 4, 2023 14:20
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

@coveralls
Copy link

coveralls commented Jul 4, 2023

Pull Request Test Coverage Report for Build 5604254441

  • 18 of 21 (85.71%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.007%) to 86.076%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/dagcircuit/dagcircuit.py 18 21 85.71%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
qiskit/circuit/tools/pi_check.py 1 91.23%
qiskit/pulse/library/waveform.py 3 93.75%
crates/qasm2/src/lex.rs 5 90.63%
Totals Coverage Status
Change from base Build 5603596398: -0.007%
Covered Lines: 72633
Relevant Lines: 84382

💛 - Coveralls

As part of this minor refactor, this updates the logic to no longer
silently override conditions on the replacement `op`.  The method gains
a `propagate_condition` argument analogous to the same argument in
`substitute_node_with_dag`, which can be set `False` to specify that the
caller is aware that the new operation should implement the same
conditional logic.
@jakelishman jakelishman force-pushed the expr/dag-substitute-node branch from e70058f to 60cd949 Compare July 19, 2023 21:25
@jakelishman jakelishman removed the on hold Can not fix yet label Jul 19, 2023
@jakelishman
Copy link
Member Author

Rebased on main.

current_wires = {wire for _, _, wire in self.edges(node)}
new_wires = set(node.qargs) | set(node.cargs)
if (new_condition := getattr(op, "condition", None)) is not None:
new_wires.update(condition_resources(new_condition).clbits)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a mapping or check that needs to be done here to ensure op.condition exists in the dag already?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you mean the wires, it happens lower down, I think: https://github.com/Qiskit/qiskit-terra/blob/60cd949fc41915c1a12fb8faa3e4c1ea3e6f94b5/qiskit/dagcircuit/dagcircuit.py#L1331-L1337.

(Looks like there might be a comment that's out of date there though haha!)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah that'll do it. It just slipped my mental context that this would validate wires.

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

@mtreinish mtreinish enabled auto-merge July 19, 2023 21:53
@mtreinish mtreinish added this pull request to the merge queue Jul 20, 2023
Merged via the queue into Qiskit:main with commit 10e66ff Jul 20, 2023
@jakelishman jakelishman deleted the expr/dag-substitute-node branch July 20, 2023 08:16
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 Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Expr to DAGCircuit.substitute_node
5 participants