Add random circuit with graph

[MozammilQ]

Summary

The function qiskit.circuit.random.utils.random_circuit_from_graph adds a new feature
of generating random circuits with interaction graph.

Details and comments

[qiskit-bot]

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 following people are relevant to this code:

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 13156564577

Details

• 130 of 133 (97.74%) changed or added relevant lines in 2 files are covered.
• 17 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.002%) to 88.665%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/random/utils.py	129	132	97.73%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	1	92.97%
crates/qasm2/src/lex.rs	4	91.98%
crates/qasm2/src/parse.rs	12	97.15%

Totals
Change from base Build 13154732632:	-0.002%
Covered Lines:	79064
Relevant Lines:	89172

---

💛 - Coveralls

[sbrandhsn]

Dear @MozammilQ thanks for showing interest in this issue. I will be on vacation and look into this PR next week! In the meantime make yourself familiar with https://github.com/Qiskit/qiskit/blob/main/CONTRIBUTING.md and the unitaryhack rules (if you have not already). :-)

[MozammilQ]

I have added the suggestions :)

[MozammilQ]

@sbrandhsn , please see if this is good enough :)

[sbrandhsn]

@MozammilQ thanks for your interest in looking at issue #12364 ! I agree this issue is a bit tricky but I think your contribution is going into the right direction. :-) Also, thanks for exploring different options for that issue!

I did not have time yet to look into your code but wanted to spend the time to clarify some of your questions. To give a little bit of context, the output of this procedure will likely not be executed directly on a quantum computer but will prove valuable for investigating compiler capabilities and efficiencies. The problem I wanted to fix with the depth parameter is that the given input graph may not be observed by random_circuit_from_graph otherwise. Likely the name of depth is not optimal and should be replaced by something like min_2q_gate_per_edge.

You rightfully noted that the generated circuits may become quite large but I think this is fine. I suspect that if you have N edges in the input graph and uniformly draw 10*N random edges from your input graph, you are pretty much guaranteed to have drawn every edge at least one. While this implies that the circuit depth of the random circuit can be 10*N occasionally, the circuit depth should be much smaller in practice, depending on the structure of the input graph. If an user decides to assign non-uniform weights to the input graph edges, they will need to accept that the returned random circuit will be deeper.

I think it would be a good idea to raise an error with a reasonable error message if some of the edge weights are none while others are not none. If all edge weights are none, you can assume the weight of each edge to be 1/N.

With respects to resets and conditional operations, you can simply follow the approach given in the original random_circuit function, i.e. meaning make some operations conditional with some probability and regard resets as single-qubit gates.

Some of your 'reviewer markers' should actually be comments in your code, it would be great if you add those as you see fit. :-)

I think the implementation should eventually consider the edge weight as well as consider the depth/min_2q_gate_per_edge parameter, e.g. guarantee that the returned random circuit actually has the interaction graph specified by interaction_graph when depth/min_2q_gate_per_edge is set to one.

I like the idea to fill unused qubits in a layer with a single-qubit gate if insert_1q_oper is set and otherwise do not use single-qubit gates at all. However, a layer should consist of multiple two-qubit gates (even though the number of two-qubit gates does not have be maximal in each layer). You are also right about max_operands - it is confusing in the way the issue is currently stated. In the future, one might want to include 3-qubit or 4-qubit gates but currently for this issue only two-qubit gates and optional single-qubit gates should be used.

Let me know if things became clearer and whether you require any other information. Also, if you make modifications to your PR, let me know for which implementation you went for eventually. :-)

[MozammilQ]

I have already worked a lot on this PR, so I will surely do some modifications to the code, but, actually right now bit busy in "IBM Quantum Challenge 2024"
will come back to it, after 4 days :)

@sbrandhsn , you explained it very very well, thanks a lot for the time you gave to write and explain these.

[sbrandhsn]

Absolutely! :-)

[MozammilQ]

This is python so, focus is on code readability.
Therefore, I am leaving some code repetetions deliberately :)

@sbrandhsn , please have a look, and let me know what to change.
I think reading the docstring will suffice for the explanation of the way the logic works in this PR.

[sbrandhsn]

Thanks @MozammilQ, this already looks like it is in good shape! :-) I left a couple of comments. I also think there should be a couple of more test cases with different types of interaction graphs such as sparse graphs, (almost) fully connected and a couple more random ones. Also, I think it would be best if you reused the code in other test cases instead of copying them. See https://networkx.org/documentation/stable/reference/generators.html for how to generate these graphs.

[sbrandhsn]

You can replace this chunk of code by build_interaction_graph in https://github.com/Qiskit/qiskit/blob/main/qiskit/transpiler/passes/layout/vf2_utils.py and a subgraph isomorphism check. :-)

[MozammilQ]

I have kept the logic untouched, because for some reasons build_interaction_graph is giving out a coupling map, which is not correct for the given circuit.
On visual inspection, it was confirmed that the coupling map given out by this logic is indeed correct and serve the purpose.

[MozammilQ]

@sbrandhsn , I believe the PR is already inline with your interests.

I have just removed rel_diff.

Please, have a look and tell me what to change now :)
It looks like, I am already not eligible for any bounty, so take your time :)

[sbrandhsn]

Thanks @MozammilQ ! I left a couple of comments - it would be great if you can address them. I like that you improved the code with a lot of comments compared to my last review!

[sbrandhsn]

I don't understand this section. If you add gates to qubit-pairs that are not present in the particular iteration, you would end up with a completely dense layer where some qubit-pairs might not be part of the input interaction graph, right?

[MozammilQ]

1>> extract qubit-pairs from input interaction graph

2>> Iterate over these qubit-pairs, apply 2Q gates but make sure those 2Q gates are
only applied to those qubit-pairs which are idle(both control and target), in case
any control, target found not to be idle, skip to another qubit-pair.

3>> Now, if any qubit still remains idle, apply 1Q if insert_1Q_oper is enabled.

A qubit which has only reset applied to it is considered idle.
Anyways, provided a better explanation in the docstring.

[sbrandhsn]

I think this branch is not necessary, you can use the logic for conditionals and layer_idx>0.

[MozammilQ]

This would led to the calculation of the boolean list of conditionals, even if conditionals is not required :)
This would add a tiny bit of overhead which could have been avoided if this block doesn't run at all for conditional set to False.
That is why I had kept the two separate blocks :)

Anyways, change the code to your liking :)

[sbrandhsn]

Could this line be the while condition in line 285? If yes, you can remove variable stop and make the condition clearer with a comment in line 284.

[MozammilQ]

I wanted to achieve a do-while loop behavior,
There is no need to check the condition for the first time, because its certain that all the qubits would be idle.
That's why I had checked the condition at the very last, and to make the code look pretty used a variable stop.

Anyways, changed the code to your liking :)

[MozammilQ]

I am doing code duplication because I want to take a test cp_map with zero weight for this test only.

[MozammilQ]

Choosing this logic over "pydi_graph.remove_edge"

[MozammilQ]

@sbrandhsn , please see if its good enough :)

[MozammilQ]

I have chosen not to return gates as np.ndarray, because it could happen that _get_gates() is used in some future functions where ndarray is not required?

[MozammilQ]

This is not efficient, it calls the same function as many times as the number of qubits in the gates are allowed.

I still have kept the changes because my opinion is that all the standard gates and closely related stuffs should be in one place, and any changes to them should reflect in all corners of Qiskit.

I am thinking of doing some changes to qiskit.circuit.library.standard_gates.__init__.py but that should be out of scope of this PR.

Now, regarding that 1 failing test, If I do not use get_standard_gate_name_mapping and do it in the old way, it passes.
But, If I use get_standard_gate_name_mapping and change instr.param, or even if do instr.__init__(*args) the test fails, it used to pass couple of months ago, but I don't know why when qpy loads the circuit the parameters changes somehow, and that fails the test.

@eliarbel , expecting your valuable comments :)

[eliarbel]

Looking a bit at the failure, I'm afraid that your instruction-based implementation doesn't play well here with the singleton and caching models implemented for circuit data in the Rust space. In particular, the parameter updates of mutable instructions like here:

qiskit/qiskit/circuit/random/utils.py

Line 620 in 7cb79aa

current_instr.params = [int(parameters[p_start:p_end][0])]

end up with the instructions cached in Rust inconsistent with what you see on the Python side. Furthermore, QPY traverses the QuantumCircuit's Rust data structure, hence the difference.

I think the best path forward is to resort to the implementation paradigm we had before in random_circuit , i.e. with the Gate classes lists. Practically, I would revert the changed in random_circuit and change the implementation in random_circuit_from_graph to follow a similar approach. In general BTW I liked your idea of pulling the standard gate lists from one place (as with get_standard_gate_name_mapping), but this is something we can address in a separate PR.

[MozammilQ]

@eliarbel , done with the changes, Please, see if the PR is good enough now :)

[MozammilQ]

@sbrandhsn , I humbly request you to have a look at this one, only if you have time to do so :)

[MozammilQ]

@sbrandhsn , I humbly request you to have a look at this one, only if you have time to do so :)

[MozammilQ]

@sbrandhsn , @1ucian0 , things are being converted to rust these days, is that the reason the PR is on hold? If, yes should I go ahead and port it to rust?
If, not please have a look at this PR :)

I will correct these errors if this PR is required anymore :)

[eliarbel]

Thanks @MozammilQ for your patience regarding a follow-up review.
Overall this seems in line with the spec in #12364. There are some changes required before it can be merged, please see my line comments.

Also:

1. Regarding testing: I'm missing some validation related to the probabilities aspect. Can you come up with a test(s) (possibly using a fixed seed) to make sure 2Q gate distribution is within a reasonable expectancy w.r.t to the input probabilities?
2. Performance: for very large circuits, how is the random_circuit_from_graph performance compared to random_circuit?

[eliarbel]

I'm wondering, do we want to exclude an edge from being assigned a random 2q gate once the min_2q_gate_per_edge constraint is satisfied for it? Maybe as an option? Not sure what is the required behavior here, so this can be ignored.

[MozammilQ]

@eliarbel , I have done with the changes, please do have a look :)

1. Regarding testing: I'm missing some validation related to the probabilities aspect.
   Can you come up with a test(s) (possibly using a fixed seed) to make sure 2Q
   gate distribution is within a reasonable expectancy w.r.t to the input probabilities?
2. Performance: for very large circuits, how is the random_circuit_from_graph
   performance compared to random_circuit?

The test has been added in 6e7ede8

Below is the performance comparison :)
I am comparing the performance based on time taken to generate same number of operations.

from qiskit.circuit.random import random_circuit_from_graph, random_circuit
import numpy as np
import rustworkx as rx
import time

num_qubits = 5
h_h_g = rx.generators.directed_heavy_hex_graph(d=num_qubits, bidirectional = False)
seed = 32434
rng = np.random.default_rng(seed = seed)
cp_map_list = []
edge_list = h_h_g.edge_list()

# generating a non-normalized list of integers.
random_probs = rng.choice(range(10, 25), size = len(edge_list)).tolist()
sum_probs = sum(random_probs)

for idx, qubits in enumerate(edge_list):
    ctrl, trgt = qubits
    cp_map_list.append((ctrl, trgt, random_probs[idx]))

h_h_g.clear_edges()
h_h_g.add_edges_from(cp_map_list)

start_time = time.time()
qc = random_circuit_from_graph(h_h_g,
                               min_2q_gate_per_edge = 262,
                               max_operands = 2,
                               measure = True,
                               conditional = True, # Just making it a bit more challenging.
                               reset = True,
                               seed = seed,
                               insert_1q_oper = True,
                               prob_conditional = 0.91,
                               prob_reset = 0.50)
end_time = time.time()
count_oper = sum(qc.count_ops().values())
print(f"For `random_circuit_from_graph`:\nTime taken: {end_time - start_time} to generate {count_oper} operations")
print(f"Time taken per operation: {(end_time-start_time)/count_oper*1000000} us\n", 40*'#', '\n')

rand_start_time = time.time()
qc_rand = random_circuit(num_qubits = 166, depth = 1200, max_operands = 2, measure = True, conditional = True, reset = True, seed = seed)
rand_end_time = time.time()
count_rand_oper = sum(qc_rand.count_ops().values())
print(f"For `random_circuit`:\nTime taken: {rand_end_time-rand_start_time} to generate {count_rand_oper} operations")
print(f"Time taken per operation: {(rand_end_time-rand_start_time)/count_rand_oper*1000000} us")

Output

For `random_circuit_from_graph`:
Time taken: 15.235646486282349 to generate 3206923 operations
Time taken per operation: 4.750861335392945 us
 ######################################## 

For `random_circuit`:
Time taken: 13.761157035827637 to generate 3188453 operations
Time taken per operation: 4.315935356684774 us