Extended block collection #12498
Extended block collection #12498
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 9365997199Details
💛 - Coveralls |
qiskit/dagcircuit/collect_blocks.py
Outdated
| class Block(ABC): | ||
| """ | ||
| Abstract block interface. | ||
|
|
||
| This class defines an abstract interface for reasoning about blocks of nodes | ||
| that can be collected from a :class:`~qiskit.dagcircuit.DAGCircuit` or | ||
| from a :class:`~qiskit.dagcircuit.DAGDependency` using the functionality | ||
| of the :class:`~BlockCollector` class. | ||
| """ | ||
|
|
||
| @abstractmethod | ||
| def append_node(self, node): | ||
| """ | ||
| Tries to add the given node to this block. | ||
|
|
||
| This method should implement the logic for deciding whether the given | ||
| node should be added to the current block, based on the given node and the | ||
| block so far. The method should return ``True`` if the node should be | ||
| added, and ``False`` if not. In the former case, the method should also | ||
| update the specific block implementation to take the new into account. | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def size(self): | ||
| """ | ||
| This method should return the size of the block. | ||
|
|
||
| This option is used when filtering blocks collected by | ||
| ``collect_all_matching_blocks`` based on their size | ||
| as specified by the ``min_block_size`` argument. | ||
| The size of the block depends on the specific implementation | ||
| and does not necessarily need to correspond to the number of | ||
| nodes. | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def get_nodes(self): | ||
| """ | ||
| This method should return the list of nodes in this block if possible, | ||
| and ``None`` if not. | ||
|
|
||
| This method is primarily needed for backwards compatibility. It will not | ||
| be called provided that ``collect_all_matching_blocks`` is always called with | ||
| ``output_nodes = False`` which is now the preferred usage. If this is | ||
| indeed the case, the method can return ``None``. | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def reverse(self): | ||
| """ | ||
| This method should return the block with nodes in the reversed order if possible, | ||
| and ``None`` if not. | ||
|
|
||
| This method is needed to support the ``collect_from_back = True`` option in | ||
| ``collect_all_matching_blocks``, in which case the nodes are iteratively | ||
| collected from the back of the DAG, and then their order needs to be reversed. | ||
| The method can return ``None`` if nodes will never be collected from the | ||
| back of the circuit. | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def split(self, split_blocks: bool, split_layers: bool): | ||
| """ | ||
| This method should return the list of blocks obtained by splitting | ||
| the current block if possible, and ``None`` otherwise. | ||
|
|
||
| This method is needed to support the ``split_blocks`` and ``split_layers`` | ||
| options in ``collect_all_matching_blocks``, allowing the collected block | ||
| of nodes to be split into sub-blocks. The method can return ``None`` | ||
| if blocks will never be split into sub-blocks. | ||
|
|
||
| Args: | ||
| split_blocks (bool): specifies to split the block into sub-blocks | ||
| over disconnected qubit subsets. | ||
| split_layers (bool): specifies to split the blocks into layers | ||
| of non-overlapping instructions (in other words, into depth-1 | ||
| sub-blocks). | ||
|
|
||
| """ | ||
| raise NotImplementedError |
There was a problem hiding this comment.
I like how well-documented this abstract base class is and it is well designed. One potential improvement here is clarifying the return times in order to make the return types consistent.
For methods like get_nodes, reverse, and split, which could return None, it might be more intuitive to always return a list. Here an empty list could signify the same thing as None, but it would also make the return type consistent, and thus allow the methods to be used in a wider range of contexts without having to check for None.
There was a problem hiding this comment.
I think treating an empty list of nodes as None might be misleading, and I think it's safer to return None in the case that a certain method for a given Block type is not supported. I clarified the return types in 1bab966.
| from qiskit.transpiler import Layout | ||
| from qiskit.transpiler.basepasses import TransformationPass | ||
| from qiskit.circuit.library import SwapGate | ||
|
|
||
|
|
||
| class StarBlock: | ||
| class StarBlock(Block): |
There was a problem hiding this comment.
From an architectural standpoint, I think it makes more sense for StarBlock class to be placed in the collect_blocks.py. Here are some reasons for moving it:
- Separation of concerns:
collect_blocks,py is focused on defining different types of blocks and the logic for collecting them. SinceStarBlock is a specific type of block, its definition fits naturally within this module. - Reusability: It might be useful to use
StarBlockoutside ofstar_preroutingand havingStarBlockincollect_blocks.pymakes it easier to reuse. For instance, If other passes or functionalities require star-shaped block logic, they can importStarBlockfrom a centralized module.
There was a problem hiding this comment.
This and other comments regarding star_preroute.py are no longer relevant after in 1a5ef91 I've reverted the changes to this file, in order not to interfere with #12761.
I completely agree that it would be best to create a "library" of useful and reusable block types (like DefaultBlock). I am not completely convinced that specifically StarBlock is going to be useful beyond the StarPrerouting pass, so I am not sure we should expose this particular block type. In addition, I have locally tried to improve the quality of StarPreroute pass by experimenting with different variants of the StarBlock (for example only collecting stars with non-repeating non-center nodes, the intuition being that repeating nodes prevent mapping a star to a line, or only collecting stars consisting of pairwise commuting gates). Unfortunately, I did not find anything that made StarPrerouting better on say QAOA circuits, however I do find likely that a good star block for the star prerouting pass would be very specific to this pass and thus should be packaged together with the pass.
| matching_blocks = block_collector.collect_all_matching_blocks( | ||
| filter_fn=filter_fn, | ||
| block_class=StarBlock, | ||
| output_nodes=False, | ||
| min_block_size=min_block_size, |
There was a problem hiding this comment.
Since the split method for the Starblock does not do anything at the moment, and collect_all_matching_blocks defaults split_block=True, we should add the parameter that split_blocks=False to make it clear that we are not splitting at this point.
There was a problem hiding this comment.
This also makes me a bit confused on why we did not catch a DAGCircuitError in our testing of star_preroutingduring the section of collect_all_matching_block:
if split_blocks or split_layers:
tmp_blocks = []
for block in matching_blocks:
sub_blocks = block.split(split_blocks=split_blocks, split_layers=split_layers)
if sub_blocks is None:
raise DAGCircuitError(
"The block class does not implement the method ``split``."
)
tmp_blocks.extend(sub_blocks)
matching_blocks = tmp_blocksThere was a problem hiding this comment.
Oh I see, we do not catch the DAGCircuitError here as the split method for StarBlock always returns [self], which is not None. Still we should have split_blocks=False for this section to avoid confusion and unnecessary compute.
There was a problem hiding this comment.
Yeah, I agree, the previous handling was sloppy, as you have pointed out.
| def split(self, split_blocks, split_layers): | ||
| """ | ||
| In theory, this method can be used to split a star-shaped | ||
| block into star-shaped sub-blocks. For now, this does not | ||
| do anything and simply returns the list consisting of this | ||
| very block. | ||
| """ | ||
| return [self] |
There was a problem hiding this comment.
We might want to include in the release notes that the split method for StarBlock acts as a placeholder and is not fully implemented yet.
There was a problem hiding this comment.
I agree, though this is no longer relevant at the moment.
It is better to merge 12761 first, and then think how to integrate the extended block collection into the new Rust-based flow.
|
Sorry for the late reply. I think at this point it is better to merge #12761 first, and then to think how to best make use of the extended block collection class within |
Pull Request Test Coverage Report for Build 10028033203Details
💛 - Coveralls |
|
Hi @henryzou50, can you please take another look at this PR and see what's missing? Thanks! |
Summary
This PR extends the functionality of the
BlockCollectorclass, replacing the previous PR #11614. The key new feature is that the check whether a node should be added to the current block may now depend both on thefilter_fn(which specifies which nodes should be collected) and the block collected so far. The logic of theStarPreRoutingpass has been simplified to use theBlockCollectorfunctionality instead of essentially implementing this in-place. In addition to avoiding code duplication, this also allows to extend theStarPreRoutingpass with additional options such as usingDAGDependencydag representation or collecting star-shaped blocks nodes from the back of the circuit (this is not done in this PR since it requires #12418). Note that forStarPreRoutingthe logic of whether a node can be added to the block collected so far depends on the block, i.e. whether it remains star-shaped.The new class
Blockdefines the abstract API that a block class should satisfy in order to use it for block collection. The main method isappend_nodewhich specifies whether a node can be added to the current block. The methodsizereturns the size of the block which is useful when filtering blocks based on themin_block_sizeargument. The methodget_nodesis primarily needed for backwards compatibility and should return the list of nodes in the block (however it would not be called when using block collection with a specified block class). The methodreverseneeds to be implemented to support collecting nodes from the back of the circuit. The methodsplitneeds to be implemented to support splitting collected blocks into smaller sub-blocks.The new class
DefaultBlockimplements the simple functionality that can be used when the decision of whether to add a node in the current block only depends onfilter_fnand not on the block collected so far.The block collection class and functionality are backward compatible, any code that worked before should remain working. (This is why
collect_all_matching_blocksreturns a list of blocks where each block is either a subclass ofBlockor a list of nodes). However, the previously public methodscollect_matching_blockandcollect_all_matching_blocksinStarPreRoutingwere completely removed (making this specific change not backward-compatible). It was an oversight to make this methods public in the first place, and unfortunately these methods don't make much sense outside ofBlockCollector.