Skip to content

Fix condition in BasisTranslator and change node.condition to @property #6040

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 26, 2021
Merged

Fix condition in BasisTranslator and change node.condition to @property #6040

merged 11 commits into from
Mar 26, 2021

Conversation

enavarro51
Copy link
Contributor

Summary

Fixes #6016

Details and comments

From the example in #6016, a condition could be dropped when transpiling with optimization_level=3. This was a dual problem. The first was in DAGCircuit.substitute_node where the node.condition and node.op.condition were getting out of sync. The second was in the BasisTranslator where an instance of a U2 gate was being reused, resulting in 2 U2's in the circuit either having the same condition or no condition.

As part of this change, DAGNode.condition has been changed from an attribute to an @property, which will guarantee that node.condition always matches node.op.condition for op nodes.

@enavarro51 enavarro51 requested a review from a team as a code owner March 17, 2021 14:42
@mtreinish mtreinish added this to the 0.17 milestone Mar 17, 2021
@mtreinish mtreinish added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Mar 17, 2021
mtreinish
mtreinish reviewed Mar 17, 2021
View reviewed changes
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.

Overall lgtm, just a couple quick comments and questions inline.

Also, are there places elsewhere in the code where we were doing something like

node.condition = node.op.condition

or

node.condition = foo
node.op.condition = foo

if so it might be good to update those here too.

@enavarro51
Copy link
Contributor Author

In DAGCircuit.substitute_node_with_dag, this code happens

        in_dag = input_dag
        condition = node.condition
        # the dag must be amended if used in a
        # conditional context. delete the op nodes and replay
        # them with the condition.
        if condition:
            in_dag = copy.deepcopy(input_dag)
            in_dag.add_creg(condition[0])
            to_replay = []
            for sorted_node in in_dag.topological_nodes():
                if sorted_node.type == "op":
                    sorted_node.op.condition = condition
                    to_replay.append(sorted_node)
            for input_node in in_dag.op_nodes():
                in_dag.remove_op_node(input_node)
            for replay_node in to_replay:
                in_dag.apply_operation_back(replay_node.op, replay_node.qargs,
                                            replay_node.cargs)

but I think that works fine with this change. The only other place node.condition was set was in DAGDepNode. Again think that should work fine with this.

mtreinish
mtreinish reviewed Mar 19, 2021
View reviewed changes
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 mostly LGTM now, thanks for the quick updates. I just have one small comment on the wording in the release note, once that's resolved I think this is ready.

releasenotes/notes/missing-condition-transpile-e2bfb51aa2d3aa00.yaml Outdated
Comment on lines 4 to 6
The previously deprecated ``condition`` attribute for :class:`qiskit.dagcircuit.DAGNode` has been removed.
It was deprecated in the 0.15.0 release. The :attr:`qiskit.dagcircuit.DAGNode.op.condition` attribute
should be used instead.
Copy link
Member

Choose a reason for hiding this comment

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

I would tweak this a bit, it's the condition kwarg on the constructor for DAGNode that was previously deprecated and removed. The condition attribute, ie DAGNode.condition still exists and works as expected it's just changed under the covers to be property function now that returns the inner op node's condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Got it. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the release note, but wasn't sure if there needs to be a 'use instead' in this case. Let me know if it needs refining.

kdk
kdk reviewed Mar 23, 2021
View reviewed changes
@kdk kdk self-assigned this Mar 23, 2021
kdk
kdk approved these changes Mar 26, 2021
View reviewed changes
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Thanks @enavarro51 . Looks good.

@kdk kdk added the automerge label Mar 26, 2021
@mergify mergify bot merged commit e383523 into Qiskit:master Mar 26, 2021
@enavarro51 enavarro51 mentioned this pull request Mar 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtreinish mtreinish mtreinish left review comments

@kdk kdk kdk approved these changes

Assignees

@kdk kdk

Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

Transpiler eliminates classical condition for optimization_level 3
3 participants
@enavarro51 @kdk @mtreinish