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

Promote Instruction metadata to class attributes. #3947

Closed

Conversation

kdk
Copy link
Member

@kdk kdk commented Mar 9, 2020

Summary

Currently, some useful details about each instruction or gate class are only available inside each instruction's __init__ method. e.g.

class HGate():
    def __init__(self):
        super().__init__('h', 1, [], num_ctrl_qubits=1)

meaning that when given an Instruction subclass, the only way to find out which instruction it is is to instantiate it and then query its name, num_qubits, etc. This PR promotes name, num_qubits, num_clbits, and num_parameters to be class attributes when known in advance (defaulting to None when these cannot be known until the gate is created.)

Details and comments

Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

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

generally looks good. but shouldn't these be private and accessible via getters? like someone can instantiate a Hadamard now, then change num_qubits to 2, and it does not raise an error i think.

Comment on lines +26 to +29
name = 'measure'
num_qubits = 1
num_clbits = 1

Copy link
Member

Choose a reason for hiding this comment

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

should there be num_params=0?

Comment on lines +25 to +28
name = 'reset'
num_qubits = 1
num_clbits = 0

Copy link
Member

Choose a reason for hiding this comment

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

num_params=0? Otherwise it defaults to None, which seems like by definition means "unknown number of parameters".

Comment on lines +44 to +46
name = 'initialize'
num_clbits = 0

Copy link
Member

Choose a reason for hiding this comment

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

num_params=1? Essentially this should identical to diagonal gate - both take a list of numbers.

@kdk
Copy link
Member Author

kdk commented May 19, 2020

When this is resolved, we can also remove the code to pick example gates from #4442 (comment) .

@mtreinish
Copy link
Member

@kdk is there any update on this?

@kdk
Copy link
Member Author

kdk commented Sep 14, 2021

@kdk is there any update on this?

I think this is mostly subsumed by #6883 . Once that merges, I'll revisit to see if there's anything left to be done here.

@mtreinish
Copy link
Member

I'm going to close this since it's been idle for 2.5 years and the plans around the instruction model has changed with Qiskit/RFCs#25 I think we'll need to start from scratch to do this assuming it works with the plans around operations

@mtreinish mtreinish closed this Aug 31, 2022
raynelfss added a commit to raynelfss/qiskit that referenced this pull request Sep 24, 2024
- Remove stale comment related to Qiskit#3947
- Rename tuple instances to `GateIdentifier`.
- Rename `doomed_nodes` to `nodes_to_replace`.
- Add docstring for `get_example_gates`.
- Rename `get_example_gates` to `get_gate_num_params`.

Co-authored-by: Kevin Hartman <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Sep 27, 2024
)

* Initial: Add `compose_transforms` and `get_example_gates` to Rust.

* Add: `compose_transform` logic in rust

* Fix: Correct the behavior of `compose_transforms`.
-  Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation.
- Remove panic statement when checking for example gates.
- Use qreg when adding an instruction to the mock_dag in `compose_transforms`.
- Add missing comparison check in for loop to compare the mapped instructions.
- Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract.
- Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`.

* Fix: Leverage rust-based `circuit_to_dag` converters.
- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.

* Format: Fix indentation of import lines in `compose_transforms.rs`

* Formatting: Separate complicated type aliases

* Fix: Adapt to new `DAGCircuit` limitations

* Format: Remove unused imports in BasisTranslator

* Fix: Adapt to #13143

* Fix: Code review comments
- Remove unused `circuit_to_dag` and `OperationRef` imports.
- Streamline complex types into simpler aliases.
- Rename `CircuitRep` to `CircuitFromPython`.
- Reshape `get_example_gates` to work with `CircuitData` instances during its recursion.
- Remove unnecessary clone of `gate_name` in `compose_transform`.
- Use `mapped_instructions` values when iterating in `compose_transforms`.
- Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust.
- Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple.

* Fix: More commments from code review
- Remove stale comment related to #3947
- Rename tuple instances to `GateIdentifier`.
- Rename `doomed_nodes` to `nodes_to_replace`.
- Add docstring for `get_example_gates`.
- Rename `get_example_gates` to `get_gate_num_params`.

Co-authored-by: Kevin Hartman <[email protected]>

* Refactor: Rename `example_gates` to `gate_param_counts`
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after #12585 merged.

---------

Co-authored-by: Kevin Hartman <[email protected]>
emilkovacev pushed a commit to emilkovacev/qiskit that referenced this pull request Oct 1, 2024
…kit#13137)

* Initial: Add `compose_transforms` and `get_example_gates` to Rust.

* Add: `compose_transform` logic in rust

* Fix: Correct the behavior of `compose_transforms`.
-  Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation.
- Remove panic statement when checking for example gates.
- Use qreg when adding an instruction to the mock_dag in `compose_transforms`.
- Add missing comparison check in for loop to compare the mapped instructions.
- Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract.
- Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`.

* Fix: Leverage rust-based `circuit_to_dag` converters.
- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.

* Format: Fix indentation of import lines in `compose_transforms.rs`

* Formatting: Separate complicated type aliases

* Fix: Adapt to new `DAGCircuit` limitations

* Format: Remove unused imports in BasisTranslator

* Fix: Adapt to Qiskit#13143

* Fix: Code review comments
- Remove unused `circuit_to_dag` and `OperationRef` imports.
- Streamline complex types into simpler aliases.
- Rename `CircuitRep` to `CircuitFromPython`.
- Reshape `get_example_gates` to work with `CircuitData` instances during its recursion.
- Remove unnecessary clone of `gate_name` in `compose_transform`.
- Use `mapped_instructions` values when iterating in `compose_transforms`.
- Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust.
- Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple.

* Fix: More commments from code review
- Remove stale comment related to Qiskit#3947
- Rename tuple instances to `GateIdentifier`.
- Rename `doomed_nodes` to `nodes_to_replace`.
- Add docstring for `get_example_gates`.
- Rename `get_example_gates` to `get_gate_num_params`.

Co-authored-by: Kevin Hartman <[email protected]>

* Refactor: Rename `example_gates` to `gate_param_counts`
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged.

---------

Co-authored-by: Kevin Hartman <[email protected]>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Oct 9, 2024
…kit#13137)

* Initial: Add `compose_transforms` and `get_example_gates` to Rust.

* Add: `compose_transform` logic in rust

* Fix: Correct the behavior of `compose_transforms`.
-  Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation.
- Remove panic statement when checking for example gates.
- Use qreg when adding an instruction to the mock_dag in `compose_transforms`.
- Add missing comparison check in for loop to compare the mapped instructions.
- Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract.
- Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`.

* Fix: Leverage rust-based `circuit_to_dag` converters.
- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.

* Format: Fix indentation of import lines in `compose_transforms.rs`

* Formatting: Separate complicated type aliases

* Fix: Adapt to new `DAGCircuit` limitations

* Format: Remove unused imports in BasisTranslator

* Fix: Adapt to Qiskit#13143

* Fix: Code review comments
- Remove unused `circuit_to_dag` and `OperationRef` imports.
- Streamline complex types into simpler aliases.
- Rename `CircuitRep` to `CircuitFromPython`.
- Reshape `get_example_gates` to work with `CircuitData` instances during its recursion.
- Remove unnecessary clone of `gate_name` in `compose_transform`.
- Use `mapped_instructions` values when iterating in `compose_transforms`.
- Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust.
- Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple.

* Fix: More commments from code review
- Remove stale comment related to Qiskit#3947
- Rename tuple instances to `GateIdentifier`.
- Rename `doomed_nodes` to `nodes_to_replace`.
- Add docstring for `get_example_gates`.
- Rename `get_example_gates` to `get_gate_num_params`.

Co-authored-by: Kevin Hartman <[email protected]>

* Refactor: Rename `example_gates` to `gate_param_counts`
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged.

---------

Co-authored-by: Kevin Hartman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants