-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add PEP 484 type hints to transpiler analysis and utility passes #15832
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
Changes from 3 commits
13592cc
dd4df44
8c5662d
f53935c
57e984d
2b6a512
f953847
5962863
0d614bc
e9572cf
9680725
0bc1b34
74a9466
66ee428
3507ba7
ce141dd
5dd8d15
2d5814a
467e5f4
fbf4347
1090259
5c44af7
5c40f3c
2f6dd11
4a7a027
9438f0d
46127b0
e6b3e69
5b0d5fb
6a38dd8
573b959
f18a82f
ecc51c2
7d5a06c
b86fbd1
c8021f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,9 @@ | |
|
|
||
| """Count the operations in a DAG circuit.""" | ||
|
|
||
| from __future__ import annotations | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These imports don't seem necessary in every file, could you only add them when required?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced |
||
|
|
||
| from qiskit.dagcircuit import DAGCircuit | ||
| from qiskit.transpiler.basepasses import AnalysisPass | ||
|
|
||
|
|
||
|
|
@@ -21,10 +24,16 @@ class CountOps(AnalysisPass): | |
| The result is saved in ``property_set['count_ops']`` as an integer. | ||
| """ | ||
|
|
||
| def __init__(self, *, recurse=True): | ||
| def __init__(self, *, recurse: bool = True) -> None: | ||
| """Initialize a ``CountOps`` pass. | ||
|
|
||
| Args: | ||
| recurse: If ``True`` (default), recursively count operations | ||
| inside control-flow blocks. | ||
| """ | ||
| super().__init__() | ||
| self.recurse = recurse | ||
|
|
||
| def run(self, dag): | ||
| """Run the CountOps pass on `dag`.""" | ||
| def run(self, dag: DAGCircuit) -> None: | ||
| """Run the CountOps pass on *dag*.""" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This applies to a bunch of other places too
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed,updated to dag across all affected files. |
||
| self.property_set["count_ops"] = dag.count_ops(recurse=self.recurse) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,9 @@ | |
|
|
||
| """Automatically require analysis passes for resource estimation.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from qiskit.dagcircuit import DAGCircuit | ||
| from qiskit.transpiler.basepasses import AnalysisPass | ||
| from qiskit.transpiler.passes.analysis.depth import Depth | ||
| from qiskit.transpiler.passes.analysis.width import Width | ||
|
|
@@ -30,11 +33,16 @@ class ResourceEstimation(AnalysisPass): | |
| * Size() | ||
| * CountOps() | ||
| * NumTensorFactors() | ||
| * NumQubits() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh nice catch 👍🏻
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! 😊 |
||
| """ | ||
|
|
||
| def __init__(self): | ||
| def __init__(self) -> None: | ||
| super().__init__() | ||
| self.requires += [Depth(), Width(), Size(), CountOps(), NumTensorFactors(), NumQubits()] | ||
|
|
||
| def run(self, _): | ||
| """Run the ResourceEstimation pass on `dag`.""" | ||
| def run(self, dag: DAGCircuit) -> None: | ||
| """Run the ResourceEstimation pass on *dag*. | ||
|
|
||
| This is a no-op: the actual analysis is performed by the required | ||
| sub-passes listed in :attr:`requires`. | ||
|
Hadar01 marked this conversation as resolved.
Outdated
|
||
| """ | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,8 +10,11 @@ | |||||
| # copyright notice, and modified files need to carry a notice indicating | ||||||
| # that they have been altered from the originals. | ||||||
|
|
||||||
| """Check if a property reached a fixed point.""" | ||||||
| """Check if the DAG contains a specific instruction.""" | ||||||
|
|
||||||
| from __future__ import annotations | ||||||
|
|
||||||
| from qiskit.dagcircuit import DAGCircuit | ||||||
| from qiskit.transpiler.basepasses import AnalysisPass | ||||||
|
|
||||||
|
|
||||||
|
|
@@ -23,23 +26,23 @@ class ContainsInstruction(AnalysisPass): | |||||
| that instruction and ``False`` if it does not. | ||||||
| """ | ||||||
|
|
||||||
| def __init__(self, instruction_name, recurse: bool = True): | ||||||
| """ContainsInstruction initializer. | ||||||
| def __init__(self, instruction_name: str | set[str], recurse: bool = True) -> None: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why change from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original docstring already documented this as str | Iterable[str] so I've matched the type hint to that , changed back to str | Iterable[str] using collections.abc.Iterable. |
||||||
| """Initialize a ``ContainsInstruction`` pass. | ||||||
|
|
||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our current suggestion is actually to just remove this initial sentence in
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, removed |
||||||
| Args: | ||||||
| instruction_name (str | Iterable[str]): The instruction or instructions to check are in | ||||||
| instruction_name: The instruction or instructions to check are in | ||||||
| the DAG. The output in the property set is set to ``contains_`` prefixed on each | ||||||
| value for this parameter. | ||||||
| recurse (bool): if ``True`` (default), then recurse into control-flow operations. | ||||||
| recurse: if ``True`` (default), then recurse into control-flow operations. | ||||||
| """ | ||||||
| super().__init__() | ||||||
| self._instruction_names = ( | ||||||
| {instruction_name} if isinstance(instruction_name, str) else set(instruction_name) | ||||||
| ) | ||||||
| self._recurse = recurse | ||||||
|
|
||||||
| def run(self, dag): | ||||||
| """Run the ContainsInstruction pass on dag.""" | ||||||
| def run(self, dag: DAGCircuit) -> None: | ||||||
| """Run the ContainsInstruction pass on *dag*.""" | ||||||
| names = dag.count_ops(recurse=self._recurse) | ||||||
| for name in self._instruction_names: | ||||||
| self.property_set[f"contains_{name}"] = name in names | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,11 @@ | |
|
|
||
| """Check if a property reached a fixed point.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from copy import deepcopy | ||
|
|
||
| from qiskit.dagcircuit import DAGCircuit | ||
| from qiskit.transpiler.basepasses import AnalysisPass | ||
|
|
||
|
|
||
|
|
@@ -25,17 +28,17 @@ class FixedPoint(AnalysisPass): | |
| as a boolean. | ||
| """ | ||
|
|
||
| def __init__(self, property_to_check): | ||
| """FixedPoint initializer. | ||
| def __init__(self, property_to_check: str) -> None: | ||
| """Initialize a ``FixedPoint`` pass. | ||
|
|
||
| Args: | ||
| property_to_check (str): The property to check if a fixed point was reached. | ||
| property_to_check: The property to check if a fixed point was reached. | ||
| """ | ||
| super().__init__() | ||
| self._property = property_to_check | ||
|
|
||
| def run(self, dag): | ||
| """Run the FixedPoint pass on `dag`.""" | ||
| def run(self, dag: DAGCircuit) -> None: | ||
| """Run the FixedPoint pass on *dag*.""" | ||
| current_value = self.property_set[self._property] | ||
| fixed_point_previous_property = f"_fixed_point_previous_{self._property}" | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it'd be best to just revert all these line removals generally 😄
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry about that! Restored the blank lines ,will keep whitespace as in the originals going forward. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's much easier to see the module docstring with this whitespace, could you leave these as they were?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that! Restored the blank lines , will keep whitespace as in the originals going forward.