Allow DAG-node substitution without overriding conditions #8615
Conversation
Currently, `DAGCircuit.substitute_node_with_dag` forcibly copies any condition on the `node` onto all the operations in the replacement DAG. This style was because the method was originally used by the basis-translation passes (`Decompose`, `BasisTranslator`, etc), which did not want to have to worry about the condition. This model of the condition being a "addition" to a node is now too restrictive with the move to more dynamic circuits; one cannot replace an (old-style) conditioned node with a custom instruction or an `IfElseBlock` that implements the condition internally, for example by taking the condition bits in the `clbits` field and using the condition on only a subset of the definition nodes. This commit does not add any such option to `DAGCircuit.substitute_node`; a new interface for wire-reordering in the arguments did not appear quite as clear to me, and this commit can stand alone.
jakelishman
added
the
Changelog: New Feature
|
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:
|
Pull Request Test Coverage Report for Build 3062224283
💛 - Coveralls |
|
This commit also speeds up the process by being a bit more CPU efficient, and eliding some register checks with new methods for handling them that are valid now that registers are aliases of qubit sequences, not owners of the qubits. I ran the That's about a 5% improvement across the board for transpiler passes that use the substitution, including the basis translator. That benchmark doesn't include any conditionals, however. I modified the This is because the condition-propagation code is done quite a bit more efficiently now. (The optimisation passes see less improvement when there are conditionals just because conditionals reduce the opportunities for All that is with no change to the basis translator - it's just a speedup of the internal |
qiskit/dagcircuit/dagcircuit.py
Outdated
| @@ -1112,93 +1018,157 @@ def replace_block_with_op(self, node_block, op, wire_pos_map, cycle_check=True): | |||
| for nd in node_block: | |||
| self._decrement_op(nd.op) | |||
|
|
|||
| def substitute_node_with_dag(self, node, input_dag, wires=None): | |||
| def _substitute_node_with_dag__wire_map(self, node, input_dag, wires, propagate_condition): | |||
There was a problem hiding this comment.
Was the __ intentional here to mark this as part of substitute_node_with_dag? I've not seen that convention before. Typically I would just nest these in substitute_node_with_dag although that's a lot of nested functions in this case as there are already a bunch.
There was a problem hiding this comment.
Yeah, I wasn't super happy with this or with them in the method - these are all helper functions, but they're long enough on their own, let alone with all the rest of the main body. I was looking for a way of marking them as explicitly helper functions - if you know of something better, I'm quite happy to change them. Unfortunately they logically mutate self using private methods (well, at least one of them does).
The other helpers in the function need to close over some values (in order to have the correct calling signature), so there's not much scope for moving them (not really any necessity).
There was a problem hiding this comment.
I inlined two of the helper functions in 82b5e3a, and gave the map_condition one a more general name (which was probably appropriate anyway).
qiskit/dagcircuit/dagcircuit.py
Outdated
| if len(wires) != len(node_wire_order): | ||
| raise DAGCircuitError(f"expected {len(node_wire_order)} wires, got {len(wires)}") | ||
| wire_map = dict(zip(wires, node_wire_order)) | ||
| if len(wire_map) != len(node_wire_order): | ||
| raise DAGCircuitError("duplicate wires") | ||
| for input_dag_wire, our_wire in wire_map.items(): | ||
| if our_wire not in self.input_map: | ||
| raise DAGCircuitError(f"{our_wire} is not in this DAG") |
There was a problem hiding this comment.
I'm a bit concerned about these error messages. I realize they're the same as in the old code but since we're refactoring this now it's a good time to look at them again. When these get raised from a private function like this they're not really actionable as the context from the inner private function is a bit opaque to users. I'm wondering if we actually need these checks or we can get away with letting things fail naturally later or maybe putting a more detailed message would help.
There was a problem hiding this comment.
Yeah, I'm not a big fan, and they just suck up CPU mostly unnecessarily. I'm happy to remove them if you're on board too. That would also mean that this function would be short enough to inline back into the main body. If that's ok by you, I can probably refactor the other two helper methods into private methods outside of DAGCircuit (and we can potentially follow up to swap the other use of DAGCircuit._map_condition in compose over to the new form).
There was a problem hiding this comment.
To the best of my knowledge, (at least some of) these date back to the initial DAGCircuit implementation.
I do think they have a use, even though they're not user-actionable errors, in that they guard against conditions which can lead to invalid or incorrect output circuits (where we drop gates, conditions, etc.). Ideally, these are conditions that shouldn't ever happen (but in practice still sometimes do). These serve as guardrails either when we're refactoring passes or when users are working with the DAGCircuit to say "you're off the well beaten path, there's a bug somewhere, etc."
I'd be in favor of removing them if we can find an alternate way of catching these cases (through restructuring APIs, adding more thorough DAG validation that can be used in testing, or otherwise), but against dropping them outright.
There was a problem hiding this comment.
Well-written tests should already catch all these errors, I think, because passes should be comparing their outputs to expected circuits. Equally, though, that means that users shouldn't be seeing these exceptions being generated other than by their own code calling DGCircuit.substitute_node_with_dag directly, so the concern about them being unactionable to users might not be so important.
I've left the error-checking in place, but inlined the helper function back into substitute_node_with_dag and added a "bit mapping invalid:" prefix to the messages to hopefully make the tracebacks and errors a little clearer, in 110757f. Happy to do a bit more, if we'd prefer.
There was a problem hiding this comment.
Yeah, I guess it comes down to what are usage expectations are for the API. For internal terra passes I agree we're unlikely to ever see this because we have sufficient testing. But it is possible (and definitely more likely) that an out of tree pass author could use the wire mapping argument incorrectly. That was really what I was thinking about for user actionability because if a user installs a bad/buggy plugin and trips these conditions I'm not sure they'd know what to do with these exceptions so low in the stack. But I guess I just talked myself into keeping the checks for that use case and the extra context you added to the error message is probably sufficient.
jakelishman
deleted the
conditional-DAGCircuit.substitute_node_with_dag
branch
Summary
Currently,
DAGCircuit.substitute_node_with_dagforcibly copies anycondition on the
nodeonto all the operations in the replacement DAG.This style was because the method was originally used by the
basis-translation passes (
Decompose,BasisTranslator, etc), whichdid not want to have to worry about the condition.
This model of the condition being a "addition" to a node is now too
restrictive with the move to more dynamic circuits; one cannot
replace an (old-style) conditioned node with a custom instruction or an
IfElseBlockthat implements the condition internally, for example bytaking the condition bits in the
clbitsfield and using the conditionon only a subset of the definition nodes.
This commit does not add any such option to
DAGCircuit.substitute_node;a new interface for wire-reordering in the arguments did not appear
quite as clear to me, and this commit can stand alone.
Details and comments
I'm making the PR because I need it for #8601, but I think it's valuable as a realignment of the importance of conditionals. When it was originally added, I believe it was considered as just an addition to a node, rather than an integral part of the operation being performed. This isn't the case in more dynamic circuits, and we need our low-level tooling to have ways of representing that.
I'm open to making another PR to add similar behaviour to
DAGCircuit.substitute_nodeas well - that would actually be more natural for #8601, but it's not as clear to me what the additions to the interface for that would be.cc: @kdk.