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 clifford gates to collect_cliffords #12750

Merged
merged 6 commits into from
Jul 11, 2024

Conversation

ShellyGarion
Copy link
Member

@ShellyGarion ShellyGarion commented Jul 10, 2024

Summary

This PR adds some Clifford gates to the CollectClifford() transpiler pass.

Details and comments

This is an immediate bugfix which does not handle the following problems:

@ShellyGarion ShellyGarion added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: quantum info Related to the Quantum Info module (States & Operators) mod: transpiler Issues and PRs related to Transpiler labels Jul 10, 2024
@ShellyGarion ShellyGarion added this to the 1.2.0 milestone Jul 10, 2024
@ShellyGarion ShellyGarion requested a review from a team as a code owner July 10, 2024 09:30
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Jul 10, 2024

Pull Request Test Coverage Report for Build 9890871127

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • 50 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.001%) to 89.893%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 6 92.37%
crates/circuit/src/dag_node.rs 7 85.58%
qiskit/transpiler/passes/basis/basis_translator.py 7 97.7%
qiskit/quantum_info/operators/symplectic/pauli.py 29 85.98%
Totals Coverage Status
Change from base Build 9871528102: -0.001%
Covered Lines: 65747
Relevant Lines: 73139

💛 - Coveralls

@alexanderivrii
Copy link
Contributor

alexanderivrii commented Jul 10, 2024

Thanks @ShellyGarion, this looks good! If we want a more general approach that supports whatever is supported by clifford_circuits, what do you think about changing the filter_fn to the following:

def _is_clifford_gate(node):
    """Specifies whether a node holds a clifford gate."""
    if getattr(node.op, "condition", None) is not None:
        return False
    is_clifford_gate = True
    try:
        Clifford(node.op)
    except QiskitError:
        is_clifford_gate = False
    return is_clifford_gate

or

def _is_clifford_gate(node):
    """Specifies whether a node holds a clifford gate."""
    if getattr(node.op, "condition", None) is not None:
        return False
    if node.op.name in clifford_gate_names:
        return True
    # test other gates
    is_clifford_gate = True
    try:
        Clifford(node.op)
    except QiskitError:
        is_clifford_gate = False
    return is_clifford_gate

My only concern with these suggestions is whether they are not prohibitively slow for large clifford circuits.

UPDATE: unfortunately the suggestions do make the pass slower: a quick experiment on 10 random circuits (created by circuits = [random_circuit(20, 1000, max_operands=4, seed=seed) for seed in range(10)]) gives

  • creating circuits: 6.13 seconds
  • name-based method: 15.97 seconds
  • Clifford(gate)-based method: 82.83 seconds

@alexanderivrii
Copy link
Contributor

another suggestion: can we use _BASIS_1Q and _BASIS_2Q defined at the end of clifford_circuits.py?

@ShellyGarion
Copy link
Member Author

@alexanderivrii - thanks for your suggestion. I also fixed random_clifford_circuit in a similar way.
I think it's a good solution, although it still does not handle certain u gates (and other 1 and 2-qubit gates) which are clifford for certain angles.

"pauli",
]
clifford_gate_names = (
list(_BASIS_1Q.keys()) + list(_BASIS_2Q.keys()) + ["clifford", "linear_function", "pauli"]
Copy link
Member Author

Choose a reason for hiding this comment

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

should we also add "permutation" here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea. I have checked and we can indeed construct a Clifford from a circuit containing PermutationGate objects, so it makes sense for the collection pass to handle these as well

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.

LGTM, thanks for doing this!

@alexanderivrii alexanderivrii added this pull request to the merge queue Jul 11, 2024
@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jul 11, 2024
@mtreinish mtreinish modified the milestones: 1.2.0, 1.1.2 Jul 11, 2024
Merged via the queue into Qiskit:main with commit 41267ec Jul 11, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Jul 11, 2024
* add clifford gates to collect_cliffords

* replace hard coded clifford names by clifford_circuit names

* move import

* replace hard coded clifford names in random_clifford_circuit

* add release notes

* add permutation to collect_clifford gate list

(cherry picked from commit 41267ec)

# Conflicts:
#	qiskit/circuit/random/utils.py
github-merge-queue bot pushed a commit that referenced this pull request Jul 11, 2024
* Add clifford gates to collect_cliffords (#12750)

* add clifford gates to collect_cliffords

* replace hard coded clifford names by clifford_circuit names

* move import

* replace hard coded clifford names in random_clifford_circuit

* add release notes

* add permutation to collect_clifford gate list

(cherry picked from commit 41267ec)

# Conflicts:
#	qiskit/circuit/random/utils.py

* Update utils.py

* Update utils.py

---------

Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* add clifford gates to collect_cliffords

* replace hard coded clifford names by clifford_circuit names

* move import

* replace hard coded clifford names in random_clifford_circuit

* add release notes

* add permutation to collect_clifford gate list
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 mod: quantum info Related to the Quantum Info module (States & Operators) mod: transpiler Issues and PRs related to Transpiler stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants