Consistent typehints for `NLocal` family #10479

Cryoris · 2023-07-24T08:01:24Z

Summary

Fixes #10456 and generally use collections.abc over typing.

Details and comments

Also makes the docs more consistent by removing the first line of the __init__ doc and the TODO in the Examples section (tracked in #10480).

qiskit-bot · 2023-07-24T08:01:32Z

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

@Cryoris
@Qiskit/terra-core
@ajavadia

Cryoris · 2023-07-24T08:05:28Z

qiskit/circuit/library/n_local/n_local.py

@@ -730,7 +715,7 @@ def parameter_bounds(self, bounds: list[tuple[float, float]]) -> None:

    def add_layer(
        self,
-        other: Union["NLocal", qiskit.circuit.Instruction, QuantumCircuit],
+        other: QuantumCircuit | qiskit.circuit.Instruction,


QuantumCircuit includes "NLocal" therefore I removed the explicit mention

coveralls · 2023-07-24T08:31:59Z

Pull Request Test Coverage Report for Build 5670996800

19 of 21 (90.48%) changed or added relevant lines in 5 files are covered.
499 unchanged lines in 20 files lost coverage.
Overall coverage increased (+0.008%) to 85.934%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/library/n_local/efficient_su2.py	6	7	85.71%
qiskit/circuit/library/n_local/two_local.py	4	5	80.0%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
qiskit/transpiler/passes/routing/sabre_swap.py	1	96.86%
qiskit/transpiler/passes/scheduling/alap.py	1	96.72%
qiskit/transpiler/passes/scheduling/asap.py	1	96.92%
qiskit/transpiler/passes/scheduling/scheduling/alap.py	1	94.87%
qiskit/transpiler/passes/scheduling/scheduling/asap.py	1	95.24%
qiskit/transpiler/passes/scheduling/scheduling/base_scheduler.py	1	94.59%
crates/qasm2/src/lex.rs	3	91.14%
qiskit/pulse/library/waveform.py	3	93.75%
qiskit/circuit/init.py	5	84.38%

Totals
Change from base Build 5631006520:	0.008%
Covered Lines:	73056
Relevant Lines:	85014

💛 - Coveralls

jakelishman · 2023-07-24T08:14:03Z

qiskit/circuit/library/n_local/efficient_su2.py

-        initial_state: Optional[Any] = None,
+        initial_state: QuantumCircuit | None = None,


Did this change deliberately?

Oh I thought I left a comment on that -- yes this is on purpose, we deprecated Any a few months ago (it's already gone in some NLocal circuits) but it seems we forgot it here 🙂 Any was mainly for backwards compatibility with old applications code so we don't need it anymore

jakelishman · 2023-07-24T09:50:50Z

qiskit/circuit/library/n_local/n_local.py

-    def _convert_to_block(self, layer: Any) -> QuantumCircuit:
+    def _convert_to_block(self, layer: typing.Any) -> QuantumCircuit:


Any means "don't type check this", whereas object is the base of all Python objects. The fact that this function takes either probably indicates that there's some usage of these objects that ought to be tightened up a bit, but it's not a big deal (and out of scope of this PR anyway).

Yes, this can probably be changed to Operation

jakelishman · 2023-07-24T09:53:15Z

qiskit/circuit/library/n_local/real_amplitudes.py

-        num_qubits: Optional[int] = None,
-        entanglement: Union[str, List[List[int]], Callable[[int], List[int]]] = "reverse_linear",
+        num_qubits: int | None = None,
+        entanglement: str | list[list[int]] | Callable[[int], list[int]] = "full",


This is a change in default.

Good catch, that change was not on purpose 😄

yes, it's always better to do "reverse_linear" and not "full" since it provides the same unitary, but with O(n) CX gates instead of O(n^2)

Cryoris · 2023-07-25T14:04:02Z

qiskit/circuit/library/n_local/two_local.py

+        num_qubits: int | None = None,
+        rotation_blocks: str
+        | type
+        | qiskit.circuit.Instruction


TwoLocal was missing Instruction as supported type

qiskit/circuit/library/n_local/efficient_su2.py

qiskit/circuit/library/n_local/two_local.py

jakelishman

Assuming this all passes now, then it's good to me.

* consistent typehints * lint * attempt to fix docs 1/? * fix accidental default change * fix lint * fix , instead of | (cherry picked from commit eda570c)

* consistent typehints * lint * attempt to fix docs 1/? * fix accidental default change * fix lint * fix , instead of | (cherry picked from commit eda570c) Co-authored-by: Julien Gacon <gaconju@gmail.com>

* consistent typehints * lint * attempt to fix docs 1/? * fix accidental default change * fix lint * fix , instead of |

Cryoris added 2 commits July 24, 2023 09:42

consistent typehints

8ad9c83

lint

cc92ae8

Cryoris requested a review from a team as a code owner July 24, 2023 08:01

Cryoris assigned mtreinish Jul 24, 2023

Cryoris added this to the 0.25.0 milestone Jul 24, 2023

Cryoris added documentation Something is not clear or an error documentation Changelog: None Do not include in changelog type: qa Issues and PRs that relate to testing and code quality and removed documentation Something is not clear or an error documentation labels Jul 24, 2023

Cryoris commented Jul 24, 2023

View reviewed changes

attempt to fix docs 1/?

8aa452c

jakelishman reviewed Jul 24, 2023

View reviewed changes

fix accidental default change

8933c08

jakelishman previously approved these changes Jul 24, 2023

View reviewed changes

jakelishman added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jul 24, 2023

jakelishman enabled auto-merge July 24, 2023 12:04

fix lint

fec9a65

Cryoris dismissed jakelishman’s stale review via fec9a65 July 25, 2023 14:03

Cryoris commented Jul 25, 2023

View reviewed changes

Cryoris commented Jul 26, 2023

View reviewed changes

qiskit/circuit/library/n_local/efficient_su2.py Outdated Show resolved Hide resolved

qiskit/circuit/library/n_local/two_local.py Outdated Show resolved Hide resolved

qiskit/circuit/library/n_local/two_local.py Outdated Show resolved Hide resolved

fix , instead of |

6739cee

jakelishman approved these changes Jul 26, 2023

View reviewed changes

jakelishman added this pull request to the merge queue Jul 26, 2023

Merged via the queue into Qiskit:main with commit eda570c Jul 26, 2023

mergify bot pushed a commit that referenced this pull request Jul 26, 2023

Consistent typehints for NLocal family (#10479)

79af418

* consistent typehints * lint * attempt to fix docs 1/? * fix accidental default change * fix lint * fix , instead of | (cherry picked from commit eda570c)

mergify bot mentioned this pull request Jul 26, 2023

Consistent typehints for NLocal family (backport #10479) #10507

Merged

to24toro pushed a commit to to24toro/qiskit-terra that referenced this pull request Aug 3, 2023

Consistent typehints for NLocal family (Qiskit#10479)

e5275d7

* consistent typehints * lint * attempt to fix docs 1/? * fix accidental default change * fix lint * fix , instead of |

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent typehints for `NLocal` family #10479

Consistent typehints for `NLocal` family #10479

Cryoris commented Jul 24, 2023 •

edited

Loading

qiskit-bot commented Jul 24, 2023

Cryoris Jul 24, 2023

coveralls commented Jul 24, 2023 •

edited

Loading

jakelishman Jul 24, 2023

Cryoris Jul 24, 2023

jakelishman Jul 24, 2023

Cryoris Jul 24, 2023

jakelishman Jul 24, 2023

Cryoris Jul 24, 2023

ShellyGarion Jul 26, 2023

Cryoris Jul 25, 2023

jakelishman left a comment

		initial_state: Optional[Any] = None,
		initial_state: QuantumCircuit \| None = None,

		def _convert_to_block(self, layer: Any) -> QuantumCircuit:
		def _convert_to_block(self, layer: typing.Any) -> QuantumCircuit:

Consistent typehints for NLocal family #10479

Consistent typehints for NLocal family #10479

Conversation

Cryoris commented Jul 24, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented Jul 24, 2023

Choose a reason for hiding this comment

coveralls commented Jul 24, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5670996800

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

Consistent typehints for `NLocal` family #10479

Consistent typehints for `NLocal` family #10479

Cryoris commented Jul 24, 2023 •

edited

Loading

coveralls commented Jul 24, 2023 •

edited

Loading