Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constructing Cliffords from quantum circuits with other Cliffords #9169

Merged
merged 21 commits into from
Jan 19, 2023

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Nov 20, 2022

Summary (updated)

In #7966 we started adding Cliffords to quantum circuits natively, without explicitly converting them to Instructions. Unfortunately, this also broke some of the functionality for circuits containing Cliffords, see #9159. This PR fixes the problem of constructing a Clifford from a quantum circuit that contains other Cliffords. Moreover, the performance of doing so is greatly increased as the code now uses Clifford's compose function instead of decomposing the inner clifford into simple gates.

In addition, the CollectCliffords transpiler pass has been upgraded to collect and combine "clifford gates", where the "clifford gates" may now also include objects of type LinearFunction, Clifford, and PauliGate.
Question: are there any other Clifford-like gates that can also be treated here?

Details and comments

Bug fix and performance improvement.

The code snippet

qc = QuantumCircuit(1)
qc.append(qi.random_clifford(1), [0])
qi.Clifford(qc)

attempts to create a Clifford object from a quantum circuit that contains another clifford cliff (created by random_clifford). I have experimented with two possible fixes for this: converting cliff to circuit (as would have been done previously when converting cliff to instruction and using its definition) vs. directly using the Clifford's compose function (which is possible because cliff is of type Clifford and not Instruction). The second approach seems to be considerably faster, for example on my laptop the following code

    width = 20
    depth = 100
    rng = np.random.default_rng(12345)
    random.seed(1234)
    qc = QuantumCircuit(width)
    for _ in range(depth):
        w = random.randint(1, width)
        qargs = random.sample(range(width), w)
        cliff = random_clifford(w, seed=rng)
        qc.append(cliff, qargs)

    start_time = time.time()
    cliff = Clifford(qc)
    end_time = time.time()
    print(f"Time = {end_time - start_time:.4f}")

takes 4.15 seconds using compose and 25.88 seconds using to_circuit. This is not surprising, since Clifford synthesis is time-consuming for large cliffords.

Update: with #7483 merged (Clifford.compose becoming incredibly faster), the above code now takes 0.14 seconds!

Extending the CollectCliffords transpiler pass.

This was very simple as the current code for Clifford(circuit) (in clifford_circuits.py) can already construct Cliffords from quantum circuit with linear functions and pauli gates.

@alexanderivrii alexanderivrii added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Nov 20, 2022
@alexanderivrii alexanderivrii added this to the 0.23.0 milestone Nov 20, 2022
@qiskit-bot
Copy link
Collaborator

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:

@coveralls
Copy link

coveralls commented Nov 20, 2022

Pull Request Test Coverage Report for Build 3961011878

  • 6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 84.939%

Totals Coverage Status
Change from base Build 3960963334: 0.001%
Covered Lines: 66646
Relevant Lines: 78463

💛 - Coveralls

@1ucian0
Copy link
Member

1ucian0 commented Nov 22, 2022

I think Clifford should be Instruction subclasses, because many parts of the code assume that's the type of circuit and dag elements. I forgot that Operation was already decided as the superclass for things to have in circuits.

@jakelishman
Copy link
Member

Luciano: no, this is precisely against the data model. Code that assumes that's the type of circuit and dag elements shouldn't, because it isn't any more. That's the point of Operation.

@alexanderivrii alexanderivrii changed the title [WIP] fixing problems for circuits containing Clifford ops Constructing Cliffords from quantum circuits with other Cliffords Dec 29, 2022
@alexanderivrii alexanderivrii marked this pull request as ready for review December 29, 2022 13:19
@alexanderivrii alexanderivrii requested review from a team and ikkoham as code owners December 29, 2022 13:19
@alexanderivrii
Copy link
Contributor Author

One more comment. Once the PR #9157 gets merged, it would make sense to extend constructing Cliffords from quantum circuits with permutations.

ShellyGarion
ShellyGarion previously approved these changes Jan 11, 2023
Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!
We still need a code owner approval to get it merged.

Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Overall LGTM. I have one minor question.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty solid to me, thanks. I just had a couple of minor points on the tests, but that's it.

For posterity: this PR isn't intended to fix all the issues raised by #9159, just a couple of them. We're still working on all of #9159, but it will likely require a lot more overhauls to how inverses, controls and some other attributes are applied to gates.

Comment on lines +561 to +563
op1 = Operator(qc)
op2 = Operator(qct)
self.assertTrue(op1.equiv(op2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests use equiv rather than == - the former normalises global phase, so == is a strong condition. Do we need the global-phase fuzziness here? If so, it seems a bit unnerving, since the transpiler is supposed to maintain the global phase entirely.

(Comment repeats for all the tests here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, your comment points to a real problem with OptimizeCliffords transpiler pass, that I should have but did not think about.

The problem is that Cliffords are only defined up to a global phase: while "in the real world" XY = iZ, when using Clifford's stabilizer/destabilizer tableaus, the multiplication by i get dropped, i.e. "in the Clifford world" XY=Z. This is also the reason why most of the tests that construct an Operator out of Clifford in test_clifford.py check for equiv and not for ==.

The above in code:

qc = QuantumCircuit(1)
qc.x(0)
qc.y(0)
print(Operator(Clifford(qc)) == Operator(qc))

print the value False.

So right now replacing a block of Clifford gates by a Clifford may drop the global phase, which is indeed the problem to "the transpiler maintaining the global phase". That is, the OptimizeCliffords transpiler pass can only guarantee equivalence up to a global phase.

Note: I do believe that when optimizing Cliffords over a subset of qubits the problem is still only with global and not local phases.

BTW, I thought there was an effort to incorporate the full phase into the Clifford class, maybe @ShellyGarion or @ikkoham can comment? This would immediately solve our problems here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I don't think we should replace equiv by == in these tests. However, we should see if there is already a solution for maintaining the global phase. And if not we should clearly document this caveat in the documentation for OptimizeCliffords transpiler pass. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a sensible plan to me. If the phase non-equivalence is already in the pass and in Terra 0.22, then there's no massive rush to fix it now before the release, and anyway, we can treat it as a bug fix. Let's open a new issue and discuss the right way forwards.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential global-phase issues aside, which are for another issue, this looks good to merge to me.

@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog automerge labels Jan 19, 2023
@jakelishman
Copy link
Member

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jan 19, 2023

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@jakelishman
Copy link
Member

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jan 19, 2023

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mtreinish
Copy link
Member

@Mergifyio unqueue

@mergify
Copy link
Contributor

mergify bot commented Jan 19, 2023

unqueue

✅ The pull request has been removed from the queue

@mtreinish
Copy link
Member

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jan 19, 2023

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify mergify bot merged commit 13bb17c into Qiskit:main Jan 19, 2023
@alexanderivrii alexanderivrii deleted the clifford-fixes branch January 20, 2023 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants