Improving CouplingGraph.get_subgraphs_of_size

[jclark-ce]

Hello,

I was using ScanPartitioner to partition some circuits, and I noticed that it performs very slowly with increasing block size even on circuits with linear connectivity. After testing CouplingGraph's get_subgraphs_of_size, I discovered that it takes a time exponential $k$ to produce the $\sim{n}$ groups of size $k$ in a linear coupling graph.

I have therefore written a small optimization which takes advantage of the fact that if the current group has been encountered before, no new groups can be found by continuing to search. The change reduces the time to find all groups of size $8$ on a 10x10 grid coupling graph from $\sim{88}$ seconds down to $\sim{2}$ seconds.

The changed version saves all qubit groups to locations as frozenset and only recurs if the new group has not been encountered before. Groups with less than size qubits are dropped before returning from get_subgraphs_of_size. The change does slightly increase the memory complexity of the method since all groups of size $\leq{k}$ are saved, although in most cases this is not an appreciable increase (since the number of groups with size $<k$ is usually less than the number of group with size $=k$).

Because groups with size $<k$ are produced for free with this method, it might also be sensible to add an interface which allows them to be left in for use cases like ScanPartitioner which need them anyway.

[edyounis]

Thanks for the PR! Awesome catch and good use of memoization. I am happy with the way this PR looks currently, but if this is a performance-critical method for your application, it may be better to go with a proper ESU algorithm:

...

        if size <= 0:
            raise ValueError(f'Nonpositive size; got {size}.')

        locations: set[CircuitLocation] = set()

        for qudit in range(self.num_qudits):
            subgraph = {qudit}
            extension = {q for q in self._adj[qudit] if q > qudit}
            self._extend_subgraph(size, subgraph, extension, qudit, locations)

        return list(locations)

    def _extend_subgraph(self, size: int, subgraph: set[int], extension: set[int], vertex: int, subgraphs: set[CircuitLocation]):
        if len(subgraph) == size:
            subgraphs.add(CircuitLocation(list(subgraph)))
            return

        while len(extension) > 0:
            w = extension.pop()
            new_extension = set(extension)
            for neighbor in self._adj[w]:
                if neighbor > vertex and neighbor not in subgraph and neighbor not in extension:
                    if all(neighbor_neighbor not in subgraph or neighbor_neighbor == w for neighbor_neighbor in self._adj[neighbor]):
                        new_extension.add(neighbor)

            subgraph.add(w)
            self._extend_subgraph(size, subgraph, new_extension, vertex, subgraphs)
            subgraph.remove(w)

There's probably a better way to write that, too, but it gives an additional factor of ~2 speedup and is more memory efficient.

Another factor of ~2 speedup can be had from skipping the CircuitLocation type safety checks. For this, we would guard all the type checks in CircuitLocation's initializer by some additional optional boolean parameter check_type. BQSKit's types are extremely heavy on type checks and safety, but not necessary in this case because we can internally verify they are correct.

Because groups with size < k are produced for free with this method, it might also be sensible to add an interface that allows them to be left in for use cases like ScanPartitioner, which need them anyway.

I am all for this. If you want to add get_subgraphs_of_upto_size and then change get_subgraphs_of_size to just call that. That would be awesome.

[jclark-ce]

You make a good point regarding the ESU algorithm. I wrote the PR the way I did because it was relatively simple and made sense for ScanPartitioner. Perhaps a good compromise would be to use ESU with a flag like include_smaller on _extend_subgraph and get_subgraphs_of_size which changes the set size equality to less than or equal?

Secondarily, regarding skipping the type-safety checks, if I have spare time for it, would you be interested in another PR to implement that behavior on some of the bqskit.ir classes? For my usage of the library I just made a custom class (i.e. SimpleCircuitLocation) which didn't contain the checks, but having it built into the library would probably allow other things to be sped up as well.

[edyounis]

Perhaps a good compromise would be to use ESU with a flag like include_smaller on _extend_subgraph and get_subgraphs_of_size, which changes the set size equality to less than or equal?

👍

Secondarily, regarding skipping the type-safety checks, if I have spare time for it, would you be interested in another PR to implement that behavior on some of the bqskit.ir classes? For my usage of the library I just made a custom class (i.e. SimpleCircuitLocation) which didn't contain the checks, but having it built into the library would probably allow other things to be sped up as well.

Thank you for the thought, but honestly, unless it's an immediate performance concern for you, I wouldn't recommend this. I would still accept the PR, but when OpenQudit gets integrated into BQSKit with a 2.0 release, bqskit.ir will basically be deleted. The type checks will be substituted for a proper type system/conversions. If you have free time and have a desire to contribute, I would much rather you make a PR for your TDAG algorithm.

[jclark-ce]

Okay, I updated get_subgraphs_of_size to use a hybrid of Redelmeier's algorithm and ESU (since, from testing, Redelmeier is faster with only a tiny bit more memory usage). I also added the allow_smaller flag to CouplingGraph.get_subgraphs_of_size and MachineModel.get_locations so that applications like ScanPartitioner can use it and updated ScanPartitioner.calculate_qudit_groups to take advantage of the flag.

Regarding contributing TDAG to BQSKit, I may be interested in doing that, I'll send you an email in the next few days to discuss it.