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

Test that gateset is preserved #84

Conversation

Misty-W
Copy link
Collaborator

@Misty-W Misty-W commented Nov 13, 2024

Fixes #82 .

Adds a test to check if output circuits have gates outside of the gateset specified by the UCC BasisTranslator.

As expected, the test added here passes when CollectCliffords is commented out but otherwise fails on the random_clifford cases.

@jordandsullivan
Copy link
Collaborator

So do we know what the performance is if we convert the CliffordCollection back into 2 qubit gates? Is there still any advantage to using it?

@Misty-W
Copy link
Collaborator Author

Misty-W commented Nov 13, 2024

So do we know what the performance is if we convert the CliffordCollection back into 2 qubit gates? Is there still any advantage to using it?

Testing that out now- making a modified CollectClffords pass looks to be the most straightforward way.

@jordandsullivan
Copy link
Collaborator

So it looks like the performance is not as good as Qiskit once again?

17316107306255919874524969956

@Misty-W
Copy link
Collaborator Author

Misty-W commented Nov 14, 2024

Yes, but that's using DefaultSynthesisClifford. I'm going to try some of the other synthesis passes to see if they improve the results.

@Misty-W
Copy link
Collaborator Author

Misty-W commented Nov 14, 2024

We are still slightly worse than Qiskit in gate counts, but combining CollectCliffords and GreedySynthesisClifford does yield an improvement over UCC w/o that combination:
image
image

@jordandsullivan
Copy link
Collaborator

jordandsullivan commented Nov 14, 2024 via email

@Misty-W
Copy link
Collaborator Author

Misty-W commented Nov 15, 2024

I see. I would like to understand how Qiskit intended for the Clifford object to be used, like is there a way to just take the info it contains as classical information that we keep track of, like a Clifford frame?

That's how I originally understood the purpose of the Clifford objects, but I need to dig further into the docs to confirm if such a framework is available. Meanwhile, I've found the synthesis (e.g. Clifford synthesis) passes work hand-in-hand with the collection functions, possibly indicating the collection functions being primarily for facilitating synthesis rather than for a hybrid workflow with classical simulation. The Qiskit compilation presentation at IBM QDC also showed collection and synthesis passes working in a similar fashion.

As an alternative, we could consider additional optimizations described in the paper on which greedy Clifford synthesis is based (https://arxiv.org/abs/2105.02291) that are not implemented in the current Qiskit version.

Set `force_consolidate=True` in `ConsolidateBlocks` to reduce gate counts slightly below default qiskit
@Misty-W
Copy link
Collaborator Author

Misty-W commented Nov 15, 2024

Using the workaround suggested in Qiskit/qiskit#11659 (comment) to force consolidation in ConsolidateBlocks, such that UnitarySynthesis works more like the Symbolic Peephole Optimization proposed in https://arxiv.org/pdf/2105.02291.

So, once again UCC reigns, albeit with a narrower margin and a caveat: we allow 1q and 2q Clifford gates (not Clifford objects or gates of >2 qubits), in addition to the default target basis.

image

image

@jordandsullivan
Copy link
Collaborator

So should we maybe add a check for whether the target gateset allows Cliffords (X,Y,Z,H) to decide whether to run this set of passes?

@Misty-W
Copy link
Collaborator Author

Misty-W commented Nov 15, 2024

So should we maybe add a check for whether the target gateset allows Cliffords (X,Y,Z,H) to decide whether to run this set of passes?

I'm thinking an allow_Clifford_in_ouptut option that defaults to True.

@jordandsullivan
Copy link
Collaborator

I would prefer not to add another flag, since we already have a way to specify target gateset.

@Misty-W Misty-W requested a review from natestemen November 18, 2024 21:13
@Misty-W
Copy link
Collaborator Author

Misty-W commented Nov 18, 2024

As a follow-up to this morning's meeting, I've removed the new passes including CollectCliffords from default and just kept the added test.

I opened PR #90 for adding back the new passes in a way that preserves the natural gateset.

Copy link
Member

@natestemen natestemen left a comment

Choose a reason for hiding this comment

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

Two code clean up recommendations to help simplify the test, but looks like it's doing the right thing!

Misty-W and others added 2 commits November 19, 2024 08:21
@Misty-W Misty-W requested a review from natestemen November 19, 2024 18:09
Copy link
Member

@natestemen natestemen left a comment

Choose a reason for hiding this comment

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

Nice work! Important test to have.

Co-authored-by: nate stemen <nate@unitary.fund>
@Misty-W Misty-W merged commit 3d7e705 into main Nov 19, 2024
@Misty-W Misty-W deleted the 82-test-to-check-that-output-circuits-from-ucc-benchmarking-are-in-the-natural-gate-set branch November 19, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test to check that output circuits from ucc benchmarking are in the natural gate set
3 participants