Fix qpy support for Cliffords #11495

alexanderivrii · 2024-01-05T09:54:28Z

Summary

This PR adds QPY support for Cliffords, partially fixing #11088.

qiskit-bot · 2024-01-05T09:54:32Z

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@mtreinish
@nkanazawa1989

coveralls · 2024-01-05T10:15:43Z

Pull Request Test Coverage Report for Build 7553589438

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 (+0.005%) to 89.334%

Totals
Change from base Build 7552937080:	0.005%
Covered Lines:	59394
Relevant Lines:	66485

💛 - Coveralls

qiskit/qpy/binary_io/circuits.py

Cryoris

Maybe @mtreinish would like a final look but LGTM 🙂

mtreinish

Overall this LGTM thanks for the fix. I left one question inline about whether there is a potential simplification to the read logic.

The only other thing we maybe want to add is a backwards compatibility test to https://github.com/Qiskit/qiskit/blob/main/test/qpy_compat/test_qpy.py ? I'm not sure it's needed but if you think there's value in validating that we can load a Clifford with newer qiskit from an historical release it would be good to add there (moving forward we'd obviously have to limit the version strings, see: https://github.com/Qiskit/qiskit/blob/main/test/qpy_compat/test_qpy.py#L750).

mtreinish · 2024-01-11T22:19:57Z

qiskit/qpy/binary_io/circuits.py

@@ -290,13 +290,17 @@ def _read_instruction(
        gate_class = getattr(quantum_initializer, gate_name)
    elif hasattr(controlflow, gate_name):
        gate_class = getattr(controlflow, gate_name)
+    elif gate_name == "Clifford":
+        pass


Shouldn't this be?

Suggested change

pass

gate_class = Clifford

Mostly just for consistency. But if we did this then we would be able to drop the special handling in L302 as it would just call gate_class(*params). Or is len(Clifford.params) > 1 and doing this wouldn't work?

Done in bc1f5e3.

mtreinish · 2024-01-11T22:24:43Z

I've tagged this as stable backport potential because this is something we can fix for 0.45.x, especially as there is no version change to the QPY format.

alexanderivrii · 2024-01-15T15:03:14Z

Since you have mentioned this, I think that adding a backwards compatibility Cliffords test might be a good idea, this is done 5f42977. The only thing I am not 100% sure is whether the line limiting the version strings should read version_parts >= (0, 25, 0):.

alexanderivrii · 2024-01-16T07:20:23Z

Update: I don't understand why the backward compatibility test fails: the claimed error

  File "/home/vsts/work/1/s/test/qpy_compat/1.0.0b1/lib/python3.8/site-packages/qiskit/qpy/binary_io/circuits.py", line 581, in _write_instruction
    gate_class_name = instruction.operation.base_class.__name__
AttributeError: 'Clifford' object has no attribute 'base_class'

is exactly what was fixed in this PR. Or is this because the milestone for this PR is set to 0.45.2 but it should be 0.46.0 or something like this?

Cryoris · 2024-01-16T10:25:32Z

It looks like the compatibility tests fail because you've added a Clifford based test which is run with a Qiskit version prior to your PR. If you check the line number where the error is raised (581) that refers to the line of the code that does not exist in your PR 🙂 I'm assuming we have to enable the new Clifford test in qpy_compat/test_qpy.py only for code versions with your changes.

alexanderivrii · 2024-01-16T11:04:36Z

Thanks @Cryoris, which version then we should enable? I thought it's 0.45.2 for which the PR is tagged, but I guess it should be higher than this?

mtreinish · 2024-01-16T13:41:13Z

0.45.2 is correct assuming we're going to backport this (which I think we should). The actual issue causing the failure is because of the 1.0.0b1 release is being treated as > 0.45.2 (which it is) but doesn't contain this PR. We probably need to exclude the 1.0.0b1 release from the compat tests because it's not real and doesn't have a stable api.

mtreinish · 2024-01-16T13:58:38Z

I pushed up #11572 to address this.

mtreinish

This LGTM, thanks for the updates.

mtreinish · 2024-01-17T13:40:31Z

test/qpy_compat/test_qpy.py

@@ -640,6 +640,21 @@ def generate_layout_circuits():
    return [qc]


+def generate_clifford_circuits():
+    """Test qpy circuits with Clifford operations."""
+    from qiskit.quantum_info import Clifford


I'm not sure the runtime import is strictly needed as this is only necessary if the import doesn't exist on all the historical versions that runs this file. But in the case of Clifford that should exist in 0.18.0 (which is the oldest version tested with this, which added qpy). But it doesn't make a difference in practice.

* clifford qpy support + test * release notes * cleanup: checking if instruction is of type Instruction * review comment * review comments: consistency + remove specialized handling * adding test to test_qpy.py * changing version string to 0.45.2 --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> (cherry picked from commit 786c0e9)

* clifford qpy support + test * release notes * cleanup: checking if instruction is of type Instruction * review comment * review comments: consistency + remove specialized handling * adding test to test_qpy.py * changing version string to 0.45.2 --------- Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> (cherry picked from commit 786c0e9) Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>

alexanderivrii added 2 commits January 5, 2024 11:46

clifford qpy support + test

e6135fb

release notes

1ff5c27

alexanderivrii requested a review from a team as a code owner January 5, 2024 09:54

alexanderivrii mentioned this pull request Jan 5, 2024

Add qpy support for Cliffords and AnnotatedOperations #11088

Closed

Cryoris reviewed Jan 5, 2024

View reviewed changes

qiskit/qpy/binary_io/circuits.py Outdated Show resolved Hide resolved

cleanup: checking if instruction is of type Instruction

27ca7a6

alexanderivrii mentioned this pull request Jan 7, 2024

Fix qpy support for Annotated Operations #11505

Merged

Cryoris reviewed Jan 8, 2024

View reviewed changes

qiskit/qpy/binary_io/circuits.py Outdated Show resolved Hide resolved

Cryoris previously approved these changes Jan 8, 2024

View reviewed changes

review comment

c3398af

alexanderivrii dismissed Cryoris’s stale review via c3398af January 10, 2024 06:53

Cryoris previously approved these changes Jan 11, 2024

View reviewed changes

mtreinish reviewed Jan 11, 2024

View reviewed changes

mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: qpy Related to QPY serialization labels Jan 11, 2024

mtreinish added this to the 0.45.2 milestone Jan 11, 2024

review comments: consistency + remove specialized handling

bc1f5e3

alexanderivrii dismissed Cryoris’s stale review via bc1f5e3 January 15, 2024 14:39

adding test to test_qpy.py

5f42977

changing version string to 0.45.2

f593b8d

Merge branch 'main' into fix-qpy-clifford

ffeb381

alexanderivrii requested a review from mtreinish January 17, 2024 13:30

mtreinish approved these changes Jan 17, 2024

View reviewed changes

mtreinish added this pull request to the merge queue Jan 17, 2024

Merged via the queue into Qiskit:main with commit 786c0e9 Jan 17, 2024
13 checks passed

mergify bot mentioned this pull request Jan 17, 2024

Fix qpy support for Cliffords (backport #11495) #11582

Merged

This was referenced Jan 31, 2024

Update QPY for Qiskit 1.0 Qiskit/qiskit-ibm-provider#803

Closed

QPY Qiskit 1.0 updates Qiskit/qiskit-ibm-runtime#1365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix qpy support for Cliffords #11495

Fix qpy support for Cliffords #11495

alexanderivrii commented Jan 5, 2024

qiskit-bot commented Jan 5, 2024

coveralls commented Jan 5, 2024 •

edited

Loading

Cryoris left a comment

mtreinish left a comment

mtreinish Jan 11, 2024

alexanderivrii Jan 15, 2024

mtreinish commented Jan 11, 2024

alexanderivrii commented Jan 15, 2024

alexanderivrii commented Jan 16, 2024

Cryoris commented Jan 16, 2024

alexanderivrii commented Jan 16, 2024

mtreinish commented Jan 16, 2024

mtreinish commented Jan 16, 2024

mtreinish left a comment

mtreinish Jan 17, 2024

Fix qpy support for Cliffords #11495

Fix qpy support for Cliffords #11495

Conversation

alexanderivrii commented Jan 5, 2024

Summary

qiskit-bot commented Jan 5, 2024

coveralls commented Jan 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 7553589438

Warning: This coverage report may be inaccurate.

💛 - Coveralls

Cryoris left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Jan 11, 2024

Choose a reason for hiding this comment

alexanderivrii Jan 15, 2024

Choose a reason for hiding this comment

mtreinish commented Jan 11, 2024

alexanderivrii commented Jan 15, 2024

alexanderivrii commented Jan 16, 2024

Cryoris commented Jan 16, 2024

alexanderivrii commented Jan 16, 2024

mtreinish commented Jan 16, 2024

mtreinish commented Jan 16, 2024

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Jan 17, 2024

Choose a reason for hiding this comment

coveralls commented Jan 5, 2024 •

edited

Loading