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

Add circuit generation from set of stabilizers #11483

Merged
merged 13 commits into from Jan 18, 2024

Conversation

Randl
Copy link
Contributor

@Randl Randl commented Jan 3, 2024

  • I have added the tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

Summary

Add stabilizer_to_circuit function that given a set of stabilizers returns a (Clifford) circuit whose output (on zero input) is stabilized by this set.

Closes #11478

Details and comments

@Randl Randl requested review from ikkoham and a team as code owners January 3, 2024 10:33
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 3, 2024
@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:

  • @Qiskit/terra-core
  • @ikkoham

@coveralls
Copy link

coveralls commented Jan 3, 2024

Pull Request Test Coverage Report for Build 7541194064

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.8%) to 89.363%

Totals Coverage Status
Change from base Build 7391084016: 1.8%
Covered Lines: 59381
Relevant Lines: 66449

💛 - Coveralls

@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: quantum info Related to the Quantum Info module (States & Operators) labels Jan 3, 2024
@jakelishman jakelishman added this to the 1.0.0 milestone Jan 3, 2024
@ShellyGarion
Copy link
Member

@Randl - thank you very much for your contribution to Qiskit! Here are a few initial comments:

  • Since the main function stabilizer_to_circuit is in fact a synthesis function, then it seems that it would better fit into the synthesis library here: https://docs.quantum.ibm.com/api/qiskit/synthesis#stabilizer-state-synthesis
    To fit with the naming convention, perhaps you can call it: synth_stabilizer_from_generators ?

  • All other helper functions should be internal, and so their names should start with an underscore, e.g. add_sign should be _add_sign.

  • In addition, perhaps we can also add it as a method to the StabilizerState class? we can call it from_stabilizers_to_circuit ?
    Similar to what we did here:

  • How do you handle signs? Generally, the stabilizers could also have +/- signs (these are the phases of the Clifford tableau), which translate to the corresponding Pauli gates when you construct the circuit. I didn't see tests for this.

  • I would also be happy to see some pseudo-random tests, that check that you get back the same stabilizers, like:

cliff = random_clifford(num_qubits, seed=1234)
stabs = cliff.to_labels(mode="S")
qc = stabilizer_to_circuit(stabs)
cliff1 = Clifford(qc)
cliff1.to_labels(mode="S") # should be the same as stabs up to a permutation

See similar pseudo-random tests in: https://github.com/Qiskit/qiskit/blob/main/test/python/synthesis/test_stabilizer_synthesis.py

* Move to synthesis library and rename accordingly
* Add pseudo-random tests
* Fix docstrings
@Randl
Copy link
Contributor Author

Randl commented Jan 8, 2024

@ShellyGarion
I think I've addressed your comments

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.

Hi @Randl - I very much like your PR and think it will be an important contribution to Qiskit.
I have a few additional minor comments.
I would also like @alexanderivrii to review this PR.

qiskit/quantum_info/states/stabilizerstate.py Outdated Show resolved Hide resolved
test/python/synthesis/test_stabilizer_circuit.py Outdated Show resolved Hide resolved
qiskit/quantum_info/states/stabilizerstate.py Outdated Show resolved Hide resolved
qiskit/synthesis/stabilizer/stabilizer_circuit.py Outdated Show resolved Hide resolved
qiskit/synthesis/stabilizer/stabilizer_circuit.py Outdated Show resolved Hide resolved
test/python/synthesis/test_stabilizer_circuit.py Outdated Show resolved Hide resolved
test/python/synthesis/test_stabilizer_circuit.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Overall, this looks good.

I have left inline a few minor nitpicks about formatting and possibility for code reuse, but nothing blocking.

A quick question about the implementation: is there an advantage to working with the string representation of stabilizers (such as "+XIZI") vs. converting them to Paulis and working with the underlying nd.arrays?

Also a question about the scalability of the approach: how much time does this take to synthesize stabilizer states for 100 or so qubits?

qiskit/quantum_info/states/stabilizerstate.py Outdated Show resolved Hide resolved
Comment on lines +124 to +125
Return:
StabilizerState: a state stabilized by stabilizers.
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use Returns: instead of Return: (though I do see Return: in multiple places in the code as well). Let me address this to a higher autority, @jakelishman.

qiskit/synthesis/stabilizer/stabilizer_circuit.py Outdated Show resolved Hide resolved
qiskit/synthesis/stabilizer/stabilizer_circuit.py Outdated Show resolved Hide resolved
qiskit/synthesis/stabilizer/stabilizer_circuit.py Outdated Show resolved Hide resolved
qiskit/synthesis/stabilizer/stabilizer_circuit.py Outdated Show resolved Hide resolved
@Randl
Copy link
Contributor Author

Randl commented Jan 16, 2024

@alexanderivrii you're correct about using Pauli; it makes code cleaner. I've rewritten the algorithm removing string usage.
As for runtime, the algorithm is quadratic, but the constant is not too small. Locally, on M1 MacBook, it takes around 15-20 seconds to run a single random test (generate random Clifford + run the algorithm + check equivalence) with 100 qubits.
I've tried to address other comments, though some are irrelevant after rewrite.

Copy link
Contributor

@alexanderivrii alexanderivrii 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 for quickly addressing the previous comments (and indeed the code is now much easier to read). LGTM!

@alexanderivrii alexanderivrii added this pull request to the merge queue Jan 18, 2024
Merged via the queue into Qiskit:main with commit e341a91 Jan 18, 2024
13 checks passed
ihincks pushed a commit to ihincks/qiskit-terra that referenced this pull request Jan 19, 2024
* Add stabilizer_to_circuit function

* Fix verification test

* Mention `stabilizer_to_circuit` in `StabilizerState` documentation

* * Add `from_stabilizer_list` function to Stabilizer state
* Move to synthesis library and rename accordingly
* Add pseudo-random tests
* Fix docstrings

* doc fixes

* Allow any Collection and not just list, fix naming, tests, and docs accordingly.

* typo

* missed rename

* Documentation fixes

* fix indent

* fix indent 2

* fix indent 3

* Rewrite using `Pauli` instead of string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: quantum info Related to the Quantum Info module (States & Operators)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow to create Clifford object from list of stablizers
7 participants