Composite key for Primitives

[ikkoham]

Summary

I opened this PR for discussion.

=> Conclusion

We don’t think this is the final solution, but it will certainly be better than the current one (less collision). I would like to include this mechanism for 0.22, leaving the improvement of better proposals and QuantumCircuit objects as a future issue.

fixes #8725

Details and comments

Alternatives:

1. Use pickle
2. Use qpy

pickle is a bit faster than qpy but serialization time is unstable...

[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 the following people are requested to review this:

• @Qiskit/terra-core
• @ajavadia
• @ikkoham
• @levbishop
• @t-imamichi

[coveralls]

Pull Request Test Coverage Report for Build 3057558417

• 19 of 19 (100.0%) changed or added relevant lines in 5 files are covered.
• 92 unchanged lines in 6 files lost coverage.
• Overall coverage decreased (-0.1%) to 84.341%

---

Files with Coverage Reduction	New Missed Lines	%
src/optimize_1q_gates.rs	1	95.16%
src/sabre_swap/sabre_dag.rs	1	94.44%
qiskit/namespace.py	2	91.67%
qiskit/quantum_info/operators/pauli.py	8	0%
src/results/marginalization.rs	20	77.01%
src/sabre_swap/mod.rs	60	80.2%

Totals
Change from base Build 3056114479:	-0.1%
Covered Lines:	59361
Relevant Lines:	70382

---

💛 - Coveralls

[1ucian0]

It seems to me that you are trying to implement some sort of QuantumCircuit.__hash__. Is this correct?

[ikkoham]

True, it's similar, but I don't want to have a hash in the mutable QuantumCircuit, and I believe there is collision for malicious cases in my proposal. Instead of providing hash as an official API, I would like to use it in this way for internal implementation.
For normal hash, object id is not necessary, but for our use case, I think this would be good.

[ikkoham]

To fix #8725, let's merge this PR ASAP.

[t-imamichi]

It might be good to have more attributes. The following example generates the same value though the number of qubits and classical bits are different.

from qiskit import QuantumCircuit
from qiskit.primitives.utils import _circuit_key

def foo(n):
    qc = QuantumCircuit(n, n, name='foo')
    return qc

for i in range(5):
    print(_circuit_key(foo(i)))

output

(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)
(4367160800, 'foo', 0, 0, None)

[t-imamichi]

Another collision example

from qiskit import QuantumCircuit
from qiskit.primitives.utils import _circuit_key

def foo(n):
    qc = QuantumCircuit(1, 1, name='foo')
    qc.ry(n, 0)
    return qc

for i in range(5):
    print(_circuit_key(foo(i)))

output

(4378819168, 'foo', 1, 0, None)
(4378819168, 'foo', 1, 0, None)
(4378819168, 'foo', 1, 0, None)
(4378819168, 'foo', 1, 0, None)
(4378819168, 'foo', 1, 0, None)

[t-imamichi]

We perhaps have to enumerate QuantumCircuit.data as follows. Note that it does not take care of pulse schedule.

from qiskit import QuantumCircuit
from qiskit.primitives.utils import _circuit_key

def to_tuple(qc):
    lst = []
    for inst in qc.data:
        op = inst.operation
        lst.append(((op.name, op.num_qubits, op.num_clbits, tuple(op.params)), inst.qubits))
    return tuple(lst)

def foo(n):
    qc = QuantumCircuit(1, 1, name='foo')
    qc.ry(n, 0)
    tpl = to_tuple(qc)
    print('foo', n)
    print(tpl, hash(tpl))
    return qc

for i in range(5):
    print(_circuit_key(foo(i)))

output:

foo 0
((('ry', 1, 0, (0,)), (Qubit(QuantumRegister(1, 'q'), 0),)),) -348870723359866815
(4559362368, 'foo', 1, 0, None)
foo 1
((('ry', 1, 0, (1,)), (Qubit(QuantumRegister(1, 'q'), 0),)),) -260950897317592432
(4559362368, 'foo', 1, 0, None)
foo 2
((('ry', 1, 0, (2,)), (Qubit(QuantumRegister(1, 'q'), 0),)),) -7121263129174142289
(4559362368, 'foo', 1, 0, None)
foo 3
((('ry', 1, 0, (3,)), (Qubit(QuantumRegister(1, 'q'), 0),)),) 7158846673307123618
(4559362368, 'foo', 1, 0, None)
foo 4
((('ry', 1, 0, (4,)), (Qubit(QuantumRegister(1, 'q'), 0),)),) 4696891824434941889
(4559362368, 'foo', 1, 0, None)

[ikkoham]

pulse schedule is covered by _op_start_times

[t-imamichi]

We need some test cases of Sampler to cover #8725 and pulse schedule, for example, #8604 (comment).

[t-imamichi]

If we use QuantumCircuit.data, can we exclude id(QuantumCircuit) from the key? Do we still need it to avoid collision?

[t-imamichi]

It may also good take a plot of runtime of key generation (like #8604 (comment)) because it enumerates QuantumCircuit.data and it is likely to take more time than before. I hope the current PR is still faster than serialization.

[ikkoham]

I updated the benchmark of my first comment.

[pedrorrivero]

I also wonder whether circuit.name should be included in the key. I would agree that maybe it should be in __hash__ (although it does not seem to be part of __eq__ so it could be up for debate) but probably not for indexing circuits here, since it is not functional.

In my humble opinion, this circuit_key should be based exclusively on functional elements within QuantumCircuit, having non-functional pieces added afterwards. For instance by:

def __hash__(self):
    key = _circuit_key(self)
    return hash((self.name, self.metadata, key))

The main reason for this is to avoid re-computation (e.g. extra transpiling) for circuits which are functionally identical. Regardless of what we decide, this should be clarified explicitly in the docs (i.e. whether circuit_key is strictly functional or not).

[t-imamichi]

Is it possible to make a unit test of pulse schedule?

[ikkoham]

I added a test, but is this what you want? I am not so familiar with pulse scheduling.

[t-imamichi]

Thanks. I'm not familiar with pulse scheduling, but the test looks good to me. I will approve it.

I noticed a tricky case with custom gate. We can make a custom gate with the same name and can cheat this PR.
The following example uses ry(n) with different n value, but it is enclosed in a custom gate.
So, we have to apply decompose or transpile to unroll the custom gate.
The cost of unrolling is not cheap. So, I think we can leave it as future work.

from qiskit import QuantumCircuit
from qiskit.primitives.utils import _circuit_key

def foo(n):
    circ = QuantumCircuit(1, name='custom')
    circ.ry(n, 0)
    inst = circ.to_instruction()
    qc = QuantumCircuit(1, 1, name='foo')
    qc.append(inst, [0])
    print('foo', n)
    print(qc)
    print(qc.decompose())
    return qc

for i in range(5):
    key = _circuit_key(foo(i))
    print(hash(key), key)

output

foo 0
     ┌────────┐
  q: ┤ custom ├
     └────────┘
c: 1/══════════

     ┌───────┐
  q: ┤ Ry(0) ├
     └───────┘
c: 1/═════════

-8321684804938097808 (1, 1, 0, (((Qubit(QuantumRegister(1, 'q'), 0),), (), 'custom', ()),), None)
foo 1
     ┌────────┐
  q: ┤ custom ├
     └────────┘
c: 1/══════════

     ┌───────┐
  q: ┤ Ry(1) ├
     └───────┘
c: 1/═════════

-8321684804938097808 (1, 1, 0, (((Qubit(QuantumRegister(1, 'q'), 0),), (), 'custom', ()),), None)
foo 2
     ┌────────┐
  q: ┤ custom ├
     └────────┘
c: 1/══════════

     ┌───────┐
  q: ┤ Ry(2) ├
     └───────┘
c: 1/═════════

-8321684804938097808 (1, 1, 0, (((Qubit(QuantumRegister(1, 'q'), 0),), (), 'custom', ()),), None)
foo 3
     ┌────────┐
  q: ┤ custom ├
     └────────┘
c: 1/══════════

     ┌───────┐
  q: ┤ Ry(3) ├
     └───────┘
c: 1/═════════

-8321684804938097808 (1, 1, 0, (((Qubit(QuantumRegister(1, 'q'), 0),), (), 'custom', ()),), None)
foo 4
     ┌────────┐
  q: ┤ custom ├
     └────────┘
c: 1/══════════

     ┌───────┐
  q: ┤ Ry(4) ├
     └───────┘
c: 1/═════════

-8321684804938097808 (1, 1, 0, (((Qubit(QuantumRegister(1, 'q'), 0),), (), 'custom', ()),), None)

[ikkoham]

As my first comment,

it will certainly be better than the current one (less collision).

The assumption is that this PR is better than the existing one. If the assumption is that it is better than the current one, then it is wrong not to include the id.

In offline discussion, this PR does not give a permanent solution nor a hash of QuantumCircuit.

This PR should wait for @jyu00's review, but we merge this once since this is blocking the development of applications. It may be changed in the future, including with or without id.

[t-imamichi]

Sounds good. Thank you for the update.