Add import for classes in qiskit/quantum_info/synthesis #11672

ShellyGarion · 2024-01-30T12:25:31Z

Summary

Fix a problem that was mentioned here:
#11635 (comment)

This line:
from qiskit.quantum_info.synthesis.two_qubit_decompose import TwoQubitWeylDecomposition
will work in Qiskit 0.46 and raise a deprecation warning.

Details and comments

qiskit-bot · 2024-01-30T12:25:36Z

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

@Qiskit/terra-core
@ikkoham
@levbishop

coveralls · 2024-01-30T12:34:44Z

Pull Request Test Coverage Report for Build 7738290304

0 of 20 (100.0%) changed or added relevant lines in 4 files are covered.
11 unchanged lines in 3 files lost coverage.
Overall coverage remained the same at 86.947%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.81%
crates/qasm2/src/lex.rs	4	92.44%
crates/qasm2/src/parse.rs	6	97.15%

Totals
Change from base Build 7697180712:	0.0%
Covered Lines:	74783
Relevant Lines:	86010

💛 - Coveralls

mtreinish

I think this is the right call, just one inline suggestion. Otherwise LGTM

I think there is likely some follow up work on main to expose the various specialized two qubit decomposer classes as public and include them in the documentation.

mtreinish · 2024-01-30T13:00:25Z

qiskit/quantum_info/synthesis/two_qubit_decompose.py

+from __future__ import annotations
+import warnings
+
+from qiskit.synthesis.two_qubit.two_qubit_decompose import TwoQubitWeylDecomposition


There are a few public decomposer classes in the module. It's probably better to play it safe with:

Suggested change

from qiskit.synthesis.two_qubit.two_qubit_decompose import TwoQubitWeylDecomposition

from qiskit.synthesis.two_qubit.two_qubit_decompose import *

and expose them all.

When I try to do this change, I get the following pylint errors:

qiskit/quantum_info/synthesis/two_qubit_decompose.py:30:0: W0401: Wildcard import qiskit.synthesis.two_qubit.two_qubit_decompose (wildcard-import) qiskit/quantum_info/synthesis/two_qubit_decompose.py:30:0: W0614: Unused import(s) cmath, math, io, base64, logging, np, logger, decompose_two_qubit_product_gate, TwoQubitWeylDecomposition, TwoQubitWeylIdEquiv, TwoQubitWeylSWAPEquiv, TwoQubitWeylPartialSWAPEquiv, TwoQubitWeylPartialSWAPFlipEquiv, TwoQubitWeylControlledEquiv, TwoQubitControlledUDecomposer, TwoQubitWeylMirrorControlledEquiv, TwoQubitWeylfSimaabEquiv, TwoQubitWeylfSimabbEquiv, TwoQubitWeylfSimabmbEquiv, TwoQubitWeylGeneral, Ud, trace_to_fid, rz_array, TwoQubitBasisDecomposer, TwoQubitDecomposeUpToDiagonal, two_qubit_cnot_decompose, ClassVar, Optional, Type, QuantumRegister, QuantumCircuit, Gate, CXGate, RXGate, RYGate, RZGate, QiskitError, Operator, weyl_coordinates, transform_to_magic_basis, OneQubitEulerDecomposer, DEFAULT_ATOL and deprecate_arg from wildcard import of qiskit.synthesis.two_qubit.two_qubit_decompose (unused-wildcard-import)

I can add more classes to the import (and later also to the main branch documentation), but I think it would be better to state them specifically

Note that except of TwoQubitWeylDecomposition, TwoQubitControlledUDecomposer, TwoQubitDecomposeUpToDiagonal, all other classes are variants of TwoQubitWeylDecomposition.

A search that I did in various libraries (circuit knitting, qiskit-ibm-runtime and qiskit-ibm-provider) does not indicate the usage of other classes except of TwoQubitWeylDecomposition.

I was mostly worried about other downstream users that we don't know about, or that don't have public code anywhere that we can access. I think listing all the public classes explicitly is fine, being explicit doesn't hurt anything. It'll fix the pylint errors too.

OK, I added all the classes in the file two_qubit_decompose

I was mostly worried about other downstream users that we don't know about

Though there's loads of other specific imports that are still going to break. Here we're only fixing the two-qubit one, but before (<=0.45) people were able to import e.g. qiskit.quantum_info.synthesis.one_qubit_decompose, .qsd, .weyl, .local_invariance....

That's a fair point, on the one hand we're documenting this release as "fully compatible with the Qiskit 0.45.x releases" but with extra deprecations. But we've also documented the public interface as those that are documented. This is a weird case because qiskit.quantum_info.synthesis is a dark corner of the API where there was a ton of undocumented functionality that wasn't exposed anywhere except in the leaf modules, and the definition of the stable api only really changed after 0.45 (in preparation for 1.0.0).

I'm inclined to just err on the side of caution more here just so people don't have an excuse for not upgrading to 0.46 and add module shims back in for everything that redirects to the new location inqiskit.synthesis with a deprecation warning. (like what's done for __init__.py here already)

Cryoris · 2024-01-31T12:38:47Z

qiskit/quantum_info/synthesis/two_qubit_decompose.py

+import warnings
+
+from qiskit.synthesis.two_qubit.two_qubit_decompose import (
+    TwoQubitWeylDecomposition,


It seems to be missing the TwoQubitBasisDecomposer, or is there a reason it's not here? 🙂

Yes, there is a reason. This class already appeared in qiskit/quanutm_info init file, and was properly deprecated in #11460.
This PRs handles classes that have never properly appeared in qiskit/quanutm_info init file before.

Cryoris · 2024-01-31T12:39:31Z

qiskit/quantum_info/synthesis/two_qubit_decompose.py

+from __future__ import annotations
+import warnings
+
+from qiskit.synthesis.two_qubit.two_qubit_decompose import (


While we're at it, could we then also add the decompose_two_qubit_product_gate? It also has a docstring and might have been mistaken as "public"

I think that this function is used internally in some of the classes.

Cryoris · 2024-01-31T12:47:17Z

qiskit/quantum_info/synthesis/two_qubit_decompose.py

+from __future__ import annotations
+import warnings
+
+from qiskit.synthesis.two_qubit.two_qubit_decompose import TwoQubitWeylDecomposition


I was mostly worried about other downstream users that we don't know about

Though there's loads of other specific imports that are still going to break. Here we're only fixing the two-qubit one, but before (<=0.45) people were able to import e.g. qiskit.quantum_info.synthesis.one_qubit_decompose, .qsd, .weyl, .local_invariance....

ShellyGarion · 2024-01-31T13:09:50Z

#11672 (comment) - I think that these functions are used internally (in 2q unitary or general unitary synthesis)

mtreinish · 2024-02-01T02:10:10Z

qiskit/quantum_info/synthesis/two_qubit_decompose.py

+from qiskit.synthesis.two_qubit.two_qubit_decompose import (
+    TwoQubitWeylDecomposition,
+    TwoQubitControlledUDecomposer,
+    TwoQubitDecomposeUpToDiagonal,
+    TwoQubitWeylIdEquiv,
+    TwoQubitWeylSWAPEquiv,
+    TwoQubitWeylPartialSWAPEquiv,
+    TwoQubitWeylPartialSWAPFlipEquiv,
+    TwoQubitWeylControlledEquiv,
+    TwoQubitWeylMirrorControlledEquiv,
+    TwoQubitWeylfSimaabEquiv,
+    TwoQubitWeylfSimabbEquiv,
+    TwoQubitWeylfSimabmbEquiv,
+    TwoQubitWeylGeneral,
+)


After thinking it over some more I feel a wildcard import is the proper thing to do here. We can just tell pylint to ignore the rules it was failing before:

Suggested change

from qiskit.synthesis.two_qubit.two_qubit_decompose import (

TwoQubitWeylDecomposition,

TwoQubitControlledUDecomposer,

TwoQubitDecomposeUpToDiagonal,

TwoQubitWeylIdEquiv,

TwoQubitWeylSWAPEquiv,

TwoQubitWeylPartialSWAPEquiv,

TwoQubitWeylPartialSWAPFlipEquiv,

TwoQubitWeylControlledEquiv,

TwoQubitWeylMirrorControlledEquiv,

TwoQubitWeylfSimaabEquiv,

TwoQubitWeylfSimabbEquiv,

TwoQubitWeylfSimabmbEquiv,

TwoQubitWeylGeneral,

)

# pylint: disable=wildcard-import,unused-wildcard-import

from qiskit.synthesis.two_qubit.two_qubit_decompose import *

mtreinish

I think @Cryoris is correct that we'll want similar redirect shims for the other leaf modules that were there before. In the interest of time (since we're scheduled to release tomorrow: 2024-02-01) I can just add the extra modules in #11697 after this PR merges.

ShellyGarion · 2024-02-01T07:10:18Z

I think @Cryoris is correct that we'll want similar redirect shims for the other leaf modules that were there before. In the interest of time (since we're scheduled to release tomorrow: 2024-02-01) I can just add the extra modules in #11697 after this PR merges.

@mtreinish, @Cryoris:
I did as you suggested. I added import * from the files: two_qubit_decompose, local_invariance, weyl and qsd.
There is no need to handle one_qubit_decompose since it contains only one class which was properly documented (and no other classes or functions).
I think this is a better approach than doing it in #11697.

ShellyGarion · 2024-02-01T07:58:55Z

I think this PR handles all files that were originally under qiskit.qauntum_info.synthesis:
https://github.com/Qiskit/qiskit/tree/stable/0.45/qiskit/quantum_info/synthesis

clifford_decompose - deprecated in a previous release
cnotdihedral_decompose - deprecated in a previous release
ion_decmpose - was deprecated in Deprecate qiskit/quantum_info/synthesis and move to qiskit/synthesis #11460, and removed for qiskit 1.0
local_invariance - added here, and deprecated
one_qubit_decompose - was deprecated in Deprecate qiskit/quantum_info/synthesis and move to qiskit/synthesis #11460, and moved to qiskit.synthesis
qsd - added here, and deprecated
quaternion - moved to qiskit.quantum_info in Deprecate qiskit/quantum_info/synthesis and move to qiskit/synthesis #11460
one_qubit_decompose - added here, and deprecated
weyl - added here, and deprecated
xx_decompose - was deprecated in Deprecate qiskit/quantum_info/synthesis and move to qiskit/synthesis #11460, and moved to qiskit.synthesis

mtreinish

This LGTM, thanks for doing this @ShellyGarion

add import for TwoQubitWeylDecomposition

6a2c0ae

ShellyGarion added this to the 0.46.0 milestone Jan 30, 2024

ShellyGarion requested a review from Cryoris January 30, 2024 12:25

ShellyGarion requested review from alexanderivrii and a team as code owners January 30, 2024 12:25

ShellyGarion added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Deprecation Include in "Deprecated" section of changelog labels Jan 30, 2024

ShellyGarion mentioned this pull request Jan 30, 2024

Deprecate qiskit/quantum_info/synthesis and move to qiskit/synthesis (backport #11460) #11635

Merged

fix lint errors

fa47496

mtreinish reviewed Jan 30, 2024

View reviewed changes

add more classes

8cc538c

ShellyGarion changed the title ~~Add import for TwoQubitWeylDecomposition~~ Add import for classes in two_qubit_decompose Jan 31, 2024

ShellyGarion mentioned this pull request Jan 31, 2024

Add more classes for 2q synthesis to qiskit synthesis docs #11687

Merged

Cryoris reviewed Jan 31, 2024

View reviewed changes

mtreinish reviewed Feb 1, 2024

View reviewed changes

add back all imports

56d6fdc

also add qsd

b441192

style

89b0b5a

ShellyGarion changed the title ~~Add import for classes in two_qubit_decompose~~ Add import for classes in qiskit/quantum_info/synthesis Feb 1, 2024

mtreinish approved these changes Feb 1, 2024

View reviewed changes

mtreinish added Changelog: None Do not include in changelog and removed stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Deprecation Include in "Deprecated" section of changelog labels Feb 1, 2024

mtreinish added this pull request to the merge queue Feb 1, 2024

Merged via the queue into stable/0.46 with commit fd0326d Feb 1, 2024

jakelishman deleted the fix-deprecate-qinfo branch February 2, 2024 00:29

	from qiskit.synthesis.two_qubit.two_qubit_decompose import TwoQubitWeylDecomposition
	from qiskit.synthesis.two_qubit.two_qubit_decompose import *

Add import for classes in qiskit/quantum_info/synthesis #11672

Add import for classes in qiskit/quantum_info/synthesis #11672

Uh oh!

Conversation

ShellyGarion commented Jan 30, 2024

Summary

Details and comments

Uh oh!

qiskit-bot commented Jan 30, 2024

Uh oh!

coveralls commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 7738290304

💛 - Coveralls

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShellyGarion commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

ShellyGarion commented Feb 1, 2024

Uh oh!

ShellyGarion commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jan 30, 2024 •

edited

Loading

ShellyGarion commented Jan 31, 2024 •

edited

Loading

ShellyGarion commented Feb 1, 2024 •

edited

Loading