Conversation
|
@BoxiLi Although this PR is still draft (the failing tests is due to an internal server error in Coveralls), I guess the service is down. All the tests do pass. |
BoxiLi
left a comment
There was a problem hiding this comment.
LGTM. Some small comments.
Ideally, the inverse changes should be a separate PR. But all of them are reviewed now.
|
@BoxiLi Thanks for the review. This PR is draft and there might be quite a bit of changes before it gets to a mergable state, so you might have to review some parts again. |
|
@BoxiLi This PR is ready for review. You only need to review it from the commit b54d6c2 as you already reviewed the commits before that. Rebasing it on top #325 did cause the git hashes to change wasn't aware of that earlier. That's why it shows your review comment at the top even before any commits. Gate refactoring is complete, in the next set of PRs I will only be fixing the documentation. |
There was a problem hiding this comment.
Partial review:
- What is the design purpose of having a compute_qobj and get_qobj. Do we expect users to call
compute_qobjexplicitly? Or onlyRX(angle).get_qobj() - In ParametricGate,
__eq__is done by comparing the class and argument value, but hash is done by comparing id I think? Will this cause weird behaviors?
I fed the gate class part to the AI to try to understand it better. It gives me the following caveats, which I cannot properly evaluate. Posting them here. What's your opinion?
- Controlled-gate class registration is non-atomic and can leak failed classes
Metaclass registration does two inserts: first by class name, then by (target, ctrl_qubits, ctrl_value). If the second insert fails, the first remains registered even though class creation failed, leaving stale namespace entries.
References: src/qutip_qip/operations/gateclass.py:51, :57; src/qutip_qip/operations/namespace.py:69 - Namespace uniqueness is not stable after mutation
NameSpace is a dataclass where generated eq includes mutable _registry, but hash uses only name. After registering operations, equality changes. Global duplicate checks rely on set membership (if namespace in self._registry) and can be bypassed, allowing duplicate namespaces with the same name.
References: src/qutip_qip/operations/namespace.py:25, :42, :46, :87 - Cached Qobj instances are shared and mutable (medium severity, design risk)
@cache/@lru_cache return shared Qobj objects. If any caller mutates returned objects in place, cache contents are corrupted globally. This is fine only if immutability is a hard contract.
References: src/qutip_qip/operations/gates/single_qubit_gate.py:48, :374; src/qutip_qip/operations/gates/two_qubit_gate.py:74, :457
About the last one. In principle, some part of Qobj can be muted. But we never encourage users to do this. I can cause many problem. The matrix is mutable for sure.
EDIT: The matrix is immutable. Typo
We only expect the user to use
If two objects are equal (according to
Yes, it is non-atomic since we are adding two keys for a controlled gate. Nice catch regarding leakage, I will swap my order of insertion that will prevent any leakage even in the extreme case.
I will change the
Yes this did strike my mind previously. We can have a |
|
@BoxiLi I have made the changes. Please re-review. |
|
I shall merge if the PR is ready. |
|
You can go ahead with the merge |
There was a problem hiding this comment.
Pull request overview
This PR modernizes qutip_qip’s gate API by moving standard gates into a dedicated qutip_qip.operations.gates namespace, introducing new gate base classes (parametric/controlled) and a namespace registry, and updating circuits/compilers/QASM/Qiskit integration plus tests/docs to match the new model.
Changes:
- Introduce new gate architecture:
operations.gates,ParametricGate/ControlledGate, and a globalNameSpaceregistry. - Refactor core modules (circuit, compiler, QASM, Qiskit converter/backend, algorithms) to use gate classes/instances consistently.
- Update tests, docs, and typing utilities to reflect the new public API and behavior.
Reviewed changes
Copilot reviewed 81 out of 81 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_vqa.py | Update gate imports to new operations.gates layout. |
| tests/test_utils.py | Switch decomposition utility test from check_gate to valid_unitary. |
| tests/test_scheduler.py | Update controlled-gate checks and gate comparisons for new gate API. |
| tests/test_renderer.py | Replace ad-hoc controlled-gate subclasses with get_controlled_gate factory usage. |
| tests/test_qpe.py | Update factories and controlled-gate API usage for QPE tests. |
| tests/test_qiskit.py | Update converter comparisons for new parametric/controlled gate API; add rotation conversion test. |
| tests/test_qft.py | Update QFT tests to compare gate objects/classes instead of names. |
| tests/test_qasm.py | Update std-gate imports and split test_read_qasm into two tests; adjust parsing-mode test section. |
| tests/test_processor.py | Update imports and minor message formatting; adjust gate imports for new structure. |
| tests/test_phase_flip.py | Update gate import and formatting. |
| tests/test_optpulseprocessor.py | Update gate imports. |
| tests/test_noise.py | Update gate imports. |
| tests/test_model.py | Minor formatting adjustments. |
| tests/test_device.py | Update gate imports/constructors (parametric gate instantiation changes) and IDLE usage. |
| tests/test_compiler.py | Update compiler gate maps to key by gate classes; update parametric base class usage. |
| tests/test_bit_flip.py | Update CX assertions to gate object/class equality. |
| tests/decomposition_functions/test_single_qubit_gate_decompositions.py | Update gate imports and minor parametrization formatting. |
| tests/conftest.py | Convert some string formatting to f-strings. |
| src/qutip_qip/vqa.py | Update gate factory usage and input validation; support Sequence for initial parameters. |
| src/qutip_qip/utils.py | New shared helpers (valid_unitary, range checks, input normalization). |
| src/qutip_qip/typing.py | Rename list aliases to *Sequence variants; clean up exports. |
| src/qutip_qip/transpiler/chain.py | Update to use new operations.gates namespace and gate-object comparisons. |
| src/qutip_qip/qiskit/utils/target_gate_set.py | Update type annotations for Qiskit gate mapping. |
| src/qutip_qip/qiskit/utils/converter.py | Expand Qiskit→QuTiP gate mapping and update qubit-index mapping logic. |
| src/qutip_qip/qiskit/backend.py | Accept broader sequence inputs for backend run. |
| src/qutip_qip/qasm.py | Refactor QASM parsing to use new gate factories and new gate module; broaden predefined gates. |
| src/qutip_qip/pulse/pulse.py | Minor message formatting updates. |
| src/qutip_qip/pulse/evo_element.py | Minor message formatting updates. |
| src/qutip_qip/operations/utils.py | New operations utility module including expand_operator/gate_sequence_product and controlled-unitary builder. |
| src/qutip_qip/operations/std/single_qubit_gate.py | Remove legacy std gate implementation (moved to operations.gates). |
| src/qutip_qip/operations/std/init.py | Remove legacy std gate re-export module. |
| src/qutip_qip/operations/parametric.py | New parametric gate base classes and validation logic. |
| src/qutip_qip/operations/namespace.py | New namespace registry for global gate registration. |
| src/qutip_qip/operations/gates/single_qubit_gate.py | New single-qubit gate implementations (incl. new inverses/IDENTITY/IDLE). |
| src/qutip_qip/operations/gates/other_gates.py | Update miscellaneous gates to new base classes/namespace and cached get_qobj. |
| src/qutip_qip/operations/gates/init.py | New gate exports and GATE_CLASS_MAP/controlled mapping. |
| src/qutip_qip/operations/controlled.py | New controlled-gate base and get_controlled_gate factory. |
| src/qutip_qip/operations/init.py | Re-export updated operations API (factories, utils, bases). |
| src/qutip_qip/noise/relaxation.py | Update typing and formatting; switch to *Sequence aliases. |
| src/qutip_qip/device/utils.py | Tighten bool checks and simplify interpolation-kind selection. |
| src/qutip_qip/device/spinchain.py | Update docs/examples to new gate constructors/imports. |
| src/qutip_qip/device/processor.py | Update imports/types and some formatting; adjust iterable checks. |
| src/qutip_qip/device/optpulseprocessor.py | Update docs/examples and formatting. |
| src/qutip_qip/device/model.py | Update typing annotations and int checks. |
| src/qutip_qip/device/circuitqed.py | Update gate imports/examples and native-gate list. |
| src/qutip_qip/device/cavityqed.py | Update examples to new gate constructors. |
| src/qutip_qip/decompose/decompose_single_qubit_gate.py | Replace check_gate with valid_unitary; update gate constructors. |
| src/qutip_qip/decompose/_utility.py | Remove legacy check_gate helper (replaced by utils.valid_unitary). |
| src/qutip_qip/compiler/spinchaincompiler.py | Update compiler maps to gate classes and update examples. |
| src/qutip_qip/compiler/scheduler.py | Update docs/examples and CX name handling. |
| src/qutip_qip/compiler/instruction.py | Update controlled-gate predicate to new is_controlled(). |
| src/qutip_qip/compiler/gatecompiler.py | Change compiler dispatch keys to gate classes; adjust parametric detection. |
| src/qutip_qip/compiler/circuitqedcompiler.py | Update compiler map keys and gate constructors. |
| src/qutip_qip/compiler/cavityqedcompiler.py | Update compiler map keys and gate constructors; adjust rotation area. |
| src/qutip_qip/circuit/utils.py | Remove legacy circuit input validation helpers (moved to qutip_qip.utils). |
| src/qutip_qip/circuit/simulator/utils.py | Remove duplicated simulator utils (moved/merged into operations utilities). |
| src/qutip_qip/circuit/simulator/matrix_mul_simulator.py | Remove precompute-unitary option; add explicit invalid-op error. |
| src/qutip_qip/circuit/simulator/init.py | Remove re-export of deleted simulator utils. |
| src/qutip_qip/circuit/instruction.py | Allow gate operations to be either class or instance; use slots dataclasses. |
| src/qutip_qip/circuit/draw/text_renderer.py | Update controlled/parametric checks; compare gates by object/class. |
| src/qutip_qip/circuit/draw/texrenderer.py | Update controlled/parametric checks and std gate comparisons; fix type hints. |
| src/qutip_qip/circuit/draw/mat_renderer.py | Update controlled/parametric checks and std gate comparisons. |
| src/qutip_qip/circuit/draw/color_theme.py | Expand gate color palettes for new/renamed gates. |
| src/qutip_qip/circuit/draw/base_renderer.py | Use slots dataclass; adjust measure_color initialization. |
| src/qutip_qip/circuit/circuit.py | Major refactor of gate adding/validation; deprecate legacy args; switch to new gate maps. |
| src/qutip_qip/circuit/_decompose.py | Update decomposition routines to new gate constructors and comparisons. |
| src/qutip_qip/circuit/init.py | Remove legacy deprecation wrappers for Gate/Measurement re-exports. |
| src/qutip_qip/algorithms/qpe.py | Update QPE to use new unitary/controlled gate factories. |
| src/qutip_qip/algorithms/qft.py | Update QFT to new gate imports and constructors. |
| src/qutip_qip/algorithms/phase_flip.py | Update imports to new gate module. |
| src/qutip_qip/algorithms/bit_flip.py | Update imports to new gate module. |
| pytest.ini | Adjust warning filters and add pythonpath/testpaths. |
| doc/source/qip-basics.rst | Update documented gate list to match new/expanded set. |
| doc/source/apidoc/qutip_qip.operations.rst | Update autosummary entry for controlled_gate_unitary. |
| doc/pulse-paper/qft.py | Minor formatting updates for LaTeX label generation. |
Comments suppressed due to low confidence (2)
src/qutip_qip/qiskit/utils/converter.py:154
- For controlled parametric gates (e.g. CRX/CRY/CRZ/CPHASE/CQASMU), the code instantiates the gate with
gate(arg_value), wherearg_valueis a list/sequence of parameters. This will be treated as a single argument (a list), andAngleParametricGate.validate_paramswill reject it (not a Real), breaking conversion for any parametric controlled gate. Instantiate with unpacking (e.g.gate(*arg_value)) consistent with the non-controlled gate path.
src/qutip_qip/device/processor.py:1133 raise warnings.warn(...)will always fail at runtime becausewarnings.warnreturnsNone, so this becomesraise None(TypeError: exceptions must derive from BaseException). If analytical mode cannot accept noise/kwargs, raise a real exception type (e.g. ValueError/RuntimeError) and optionally emit a warning separately.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The original PR #325 was split into 2, for the previous set of changes look at that PR description.
Defined
__slots__for all the Abstract Gate classes and standard gate classes for better memory efficiency and faster lookup time.Added boolean
is_controlled,is_parametricmethod to theGateclass.Added the concept and implementation of namespaces for operations, this is done because at several places earlier gate.name was being used for comparision. Each operation (gate for now) must now be defined in a namespace or if it is a temporary gate, namespace can be set to None. This allows the user to define faulty version of standard gates or follow their own convention for certain Parametric gates. The concept of namespace is much broader. Currently the parent namespace is "std", under it only "gates" is set as of now. In future we can make other namespaces under "std" like for Arithmetic Ops, Ansatz Ops etc. For the idea of Ops look at Refactor the Gate class objects #263 (comment) Though it hasn't been implemented yet. In a way std namespace will work like a standard library for different operations and the user can also define his own implementation for operations in a different namespace.
Added caching in
get_qobjmethod for standard gates. This is done for two reasons.if you run it multiple times and check
id()for each obj, they will be different. This leads to higher memory consumption for large circuit simulations, even though all of them represent the Z gate's qobj in this case. Additonally different qobjs will each need to be garbage collected. Memory and lookup is an even larger bottleneck in VQA circuits where several times many gates share the same parameter.Added
dtypetoget_qobj(dtype="dense")method, . This might be useful if the circuit simulation in run on a different backend like JAX, CUDA.Made
Gatenon initialised by default i.e.__init__raise will raise an Error. ObviouslyParametricGateand controlled version of them are initialisable. This makes sense because target, controls have been removed from gates andx1=X(),x2=X()now represent the same thing. Also good from memory point of view.Added
Sdag,Tdag,SQRTXdag,ISWAPdag,SQRTSWAPdag,BERKELEYdagto the standard gates. They have been added to the documentation table and in circuit draw (colour config for them).Added
inversemethod forGateclass. Also implemented inverse for all standard gates and tests for the same. For controlled gates, the inverse is auto calculated based on the target gate. Closes Add a gate inversion function #301Renamed
unitary_gatetoget_unitary_gateand added checks for all the input arguments.The control value for a gate is now baked in to the control gate itself. The user can always generate his own version of controlled gate e.g.
controlled(gates.X, num_ctrl_qubits=2, control_value=0b10). For more detail check out Refactor Gate class #308 (comment)For all Controlled gates, the (target_gate, num_ctrl_qubit, ctrl_value) is used as another key in the namespace to reference to that controlled gate. This is done in order to prevent regeneration of controlled gate class for a given target gate with known (num_ctrl_qubit, ctrl_value). Since namespace is used by default, there isn't any overhead.
Renamed
controlled_gatetocontrolled_gate_unitaryand resolved the bug which didn't allow for generating the correct qobj if number of controls > 1.Added gate equivalence checking for
ParametricGate,ControlledGate. And added__eq__,__hash__in Gate metaclass as well. Replaced gate.name = "X" search with gate == X now.Corrected inconsistency between Gate and Type[Gate] from previous PRs. Gate refers to an instantiated object while type[Gate] to a Gate class or subclass in accordance with Python's typehinting.
Added several more checks in
__init_subclass__,add_gatemethod.Added tests in
test_gates.pyandtest_circuit.pymostly incorrect cases like num_qubits being negative etc. which should raise an Error. Since Gates are used through the codebase, their normal usage is already mostly covered.Maintainence and Bug Fixes:
gate_producthad a clear bug thattensor_listwas not defined but was being used, fixed that and also moved the entiregate_productlogic to operations which was split in circuit-simulator utils, operations but was always being imported from operations as per__init__even inmat_mul_simulator.py.Removed depreciated arguments, and functions which were depreciated 3 years back. This was checked in the git commit history.
Remove mutable default values from function arguments across the codebase, this is a common Python bug. Something like targets = [], can often lead to error. For details check out this blog https://medium.com/@matbrizolla/beware-of-the-hidden-trap-understanding-mutable-default-arguments-in-python-d10e8b0a7b72
Corrected several
isInstancechecks in the codebase.IterableandSequencewere being used interchangeably at several places especially in qasm. This led to several unexpected behaviour and errors during gate refactoring.Moved pytest.ini to the root, this is what pytest themselves recommend https://docs.pytest.org/en/stable/explanation/goodpractices.html#choosing-a-test-layout-import-rules . The reason was placing pytest.ini in
tests/lead to certain sys path issues, for reference check https://docs.pytest.org/en/stable/explanation/pythonpath.html. This caused an error when standard gates were designated to a namespace and pytest was trying to redefine them and reassign them to the same namespace.Edit: Some more changes
Separated IDLE (meant for Pulse level simulation) and Identity gate (standard Identity matrix).
In Pulse compiler instead of
gate.name,gatesare themselves being used as keys.Removed expand argument from
inverse()in Parametric Gates.Renamed
controlledtoget_controlled_gate.