Feat: private instructions - #217
Conversation
ThomasB-Colibri
left a comment
ThomasB-Colibri
left a comment
There was a problem hiding this comment.
Review completed against c2f4aa3d. I found one provider-dependent correctness regression and three state/API regressions that should be addressed before merge. The GitHub Actions runs for this commit are currently action_required and contain no jobs, so no CI result was available to validate the changes. GitHub does not allow the PR author to submit a formal “request changes” review, so these are submitted as review comments.
|
|
||
| for instruction in circuit.instructions + circuit.measurements: | ||
| instructions = circuit._instructions # pyright: ignore[reportPrivateUsage] | ||
| for instruction in instructions: |
There was a problem hiding this comment.
[P1] Keep measurements terminal across provider translations
_instructions preserves insertion order, so iterating it directly changes the circuit semantics when a measurement is followed by another circuit or gate. For example, QCircuit([X(0), BasisMeasure([0], shots=1)]) + QCircuit([X(0)]) used to translate as X, X, measure; this code now emits X, measure, X for Cirq (and the same change exists in Braket), while the Qiskit translator still skips measurements in its first pass and appends all of them at the end. The same MPQP circuit can therefore produce a different distribution or be rejected depending on the provider. Please keep the previous terminal-measurement normalization in every translator, or enforce terminal measurements at the QCircuit boundary and add cross-provider tests for composed circuits.
| A copy is returned so the circuit cannot be modified by mutating this | ||
| list directly. Use :meth:`add` to add instructions to the circuit. | ||
| """ | ||
| return self._instructions.copy() |
There was a problem hiding this comment.
[P2] Preserve the public instructions contract
Before this PR, instructions contained non-measurement instructions and measurements exposed measurements separately; circuit.instructions + circuit.measurements was the established pattern used throughout MPQP. Returning the unified backing list here changes that public contract and makes the same expression duplicate every measurement for downstream callers. If the goal is encapsulation, please return a shallow filtered list of non-measurement instructions and expose an explicitly named all-instructions view if needed. Otherwise this needs an intentional compatibility/deprecation plan and migration coverage rather than landing as a side effect of privatizing storage.
| """ | ||
| if isinstance(instructions, Instruction): | ||
| index = self._instructions.index(instructions) | ||
| self._pop_instruction(index) |
There was a problem hiding this comment.
[P2] Recompute dynamic circuit dimensions after removal
_pop_instruction() updates the measurement/variable indexes but never recomputes _nb_cbits or _nb_qubits. This is observable for dynamically sized circuits: after c = QCircuit([BasisMeasure([0])]); c.remove(c.measurements[0]); c.add(BasisMeasure([0])), the second measurement starts at classical bit 1 and the circuit grows to two classical bits because the removed measurement left _nb_cbits == 1. Removing the only highest-target gate similarly leaves extra qubits. Please recompute dimensions from the remaining components when the corresponding user-defined size is None, while preserving explicitly fixed sizes, and cover both measurement and gate removal.
| if isinstance(param, Expr): | ||
| params.update(param.free_symbols) | ||
| return params | ||
| return set(self._variables) |
There was a problem hiding this comment.
[P2] Avoid caching variables across mutable instruction objects
The circuit stores the exact instruction object passed to add, and the new instructions property also returns those same mutable elements. Consequently the cache can diverge without any circuit method being called: gate = Rx(theta, 0); c = QCircuit([gate]); gate.parameters[0] = phi; c.variables() still returns {theta} although the stored gate now references phi. The previous implementation scanned the current instructions and stayed coherent. Please either make/copy contained instructions so external mutation is impossible, provide a complete invalidation mechanism, or continue computing variables on demand; add a regression test using a retained gate reference.
1 participant
Make instructions list from QCircuit private