Minor improvement to quantum_info.random_clifford

[BrunoLiegiBastonLiegi]

Minor improvement to quantum_info.random_clifford.

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.84%. Comparing base (44f5b83) to head (5552d06).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1259      +/-   ##
==========================================
- Coverage   99.85%   99.84%   -0.01%     
==========================================
  Files          73       73              
  Lines       10638    10645       +7     
==========================================
+ Hits        10623    10629       +6     
- Misses         15       16       +1     

Flag	Coverage Δ
unittests	99.84% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[BrunoLiegiBastonLiegi]

Ok this is ready @renatomello

[stavros11]

common used to be a set but now it is a bool, therefore the error message defined under this line (GitHub does not let me comment there because it is not changed in this PR) will stop making sense. It will probably read:

ValueError("True qubits are both targets and controls for gate ...")

which doesn't make much sense. It should be modified accordingly.

Other than that, what is the motivation of switching _control_qubits from set to tuple? Just symmetry with _target_qubits? (not intending to say that the current code is the best, I am sure it can be simplified in many ways)

[renatomello]

Other than that, what is the motivation of switching _control_qubits from set to tuple? Just symmetry with _target_qubits? (not intending to say that the current code is the best, I am sure it can be simplified in many ways)

In [1]: %timeit set()
63.8 ns ± 4.93 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [2]: %timeit set()
55.9 ns ± 2.81 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [3]: %timeit list()
61.4 ns ± 3.49 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [4]: %timeit tuple()
28.8 ns ± 0.179 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [5]: %timeit []
23 ns ± 2.33 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [6]: %timeit ()
7.33 ns ± 0.0883 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

It's 7x faster. Even though it's in the ns scale, it makes a difference when creating tens or even hundreds of thousands of gates e.g. random_clifford for 1000+ qubits.

[renatomello]

@stavros11

Another thing that we noted is that every single qubit gate is inheriting a control_qubits attribute from the abstract Gate class. This means that every single qubit gate carries an empty tuple with it, unless the method controlled_by is used and the tuple is filled. It may be worth for single qubit gates to move the initialization of control_qubits to inside the control_qubits method. That would make the creating of gates slightly faster and the objects slightly lighter.

[stavros11]

Thanks @renatomello for the explanation regarding the set. I was not aware about this overhead. This PR looks good to me.

Another thing that we noted is that every single qubit gate is inheriting a control_qubits attribute from the abstract Gate class. This means that every single qubit gate carries an empty tuple with it, unless the method controlled_by is used and the tuple is filled. It may be worth for single qubit gates to move the initialization of control_qubits to inside the control_qubits method. That would make the creating of gates slightly faster and the objects slightly lighter.

I wouldn't expect that this adds that big of an overhead. If it does, I am fine with dropping it for single qubit gates, however, we should be a bit careful with .controlled_by because it is not good practice to add new attributes to objects outside __init__ (pylint may even complain if you do this). Then I guess .controlled_by would have to create a different gate instance for the controlled version of the gate (which has control_qubits), but I am also not sure if it's a good solution because we'd have to replicate every non-controlled gate to support that.

In any case, I would postpone to another PR, but feel free to open an issue if you think control_qubits should be dropped.