-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Move PendingDeprecations to Deprecations in the circuit library #13604
base: main
Are you sure you want to change the base?
Conversation
We could just have a single reno for the deprecation of |
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.
Thanks for taking care of this, this LGTM but it seems to only miss the catches in the tests 🙂
4eee40c
to
a76ebc7
Compare
Most of the problems are coming from an internal call to a deprecated method:
There is also some issues with
Still working on the rest. |
…chain_dirty_ancilla_action_only
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13919787623Details
💛 - Coveralls |
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.
Thanks for the massive effort. I am definitely looking forward to not see BlueprintCircuit
and MCXGrayCode
in Qiskit 3.0. I have left some questions and comments in-place.
|
||
with warnings.catch_warnings(action="ignore", category=DeprecationWarning): | ||
circuit.mcx( | ||
[q_state, qr_sum[j], qr_carry[j - 1]], | ||
qr_carry[j], | ||
qr_control, | ||
mode="v-chain", | ||
) | ||
|
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.
Do I understand correctly that we need catch_warnings
here because we are deprecating the argument mode
in QuantumCircuit.mcx(...)
? On the other hand, do we really need this -- as the whole class WeightedAdder
is being deprecated? Note that these are only questions, I am not asking for any actual changes.
with warnings.catch_warnings(action="ignore", category=DeprecationWarning): | ||
self.append(circuit.to_gate(), self.qubits) |
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.
Why do we need to catch_warnings
here?
since="1.4", | ||
additional_msg=( | ||
"Use qiskit.circuit.library.QFTGate or qiskit.synthesis.qft.synth_qft_full instead, " | ||
"for access to all previous arguments.", | ||
), | ||
pending=True, | ||
) | ||
def __init__( |
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.
Do I understand correctly that technically we do not need this deprecation message -- as we are deprecating the base BlueprintCircuit
class?
since="1.3", | ||
since="1.4", | ||
additional_msg=( | ||
"Use the adder gates provided in qiskit.circuit.library.arithmetic instead. " | ||
"The gate type depends on the adder kind: fixed, half, full are represented by " | ||
"ModularAdderGate, HalfAdderGate, FullAdderGate, respectively. For different adder " | ||
"implementations, see https://docs.quantum.ibm.com/api/qiskit/synthesis.", | ||
), | ||
pending=True, |
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.
Some other similar deprecation messages include the argument
removal_timeline="in Qiskit 3.0",
Would it be more consistent to add a similar argument here and other similar messages?
num_variable_qubits: The qubits of which the AND is computed. The result will be written | ||
num_variable_qubits: The qubits of which the OR is computed. The result will be written |
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.
nice catch (better late than never) 😄
else: | ||
self.compose(synth_mcx_n_clean_m15(len(control_qubits)), inplace=True) |
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.
same concern about correctness here
@deprecate_arg( | ||
name="mode", | ||
since="2.0", | ||
additional_msg=( | ||
"Instead, add a generic MCXGate to the circuit and specify the synthesis method " | ||
"via the ``hls_config`` in the transpilation. Alternatively, specific decompositions " | ||
"are available at https://qisk.it/mcx." | ||
), | ||
) |
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.
Shouldn't we also deprecate the argument ancilla_qubits
?
* :class:`.QuantumVolume` ``-->`` :func:`.quantum_volume` | ||
* :class:`.EvolvedOperatorAnsatz` ``-->`` :func:`.evolved_operator_ansatz` |
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.
Should we also mention the deprecation of MCX variants in the release notes, as well as arguments to QuantumCircuit.mcx(...)
?
MCXVChain(num_ctrl_qubits, True), | ||
for synth_circuit in [ | ||
synth_mcx_n_dirty_i15(num_ctrl_qubits), | ||
synth_mcx_n_clean_m15(num_ctrl_qubits), | ||
synth_mcx_1_clean_b95(num_ctrl_qubits), | ||
synth_mcx_gray_code(num_ctrl_qubits), | ||
synth_mcx_noaux_v24(num_ctrl_qubits), |
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.
Do we need this test? Previously we checked that adding custom MCX variants worked correctly, but now we are checking correctness of synthesis methods, and this does not seem the right place to do that, do you agree?
corrected = np.zeros(2 ** (num_ctrl_qubits + 1), dtype=complex) | ||
for i, statevector_amplitude in enumerate(statevector): | ||
i = int(bin(i)[2:].zfill(circuit.num_qubits)[gate.num_ancilla_qubits :], 2) | ||
i = int(bin(i)[2:].zfill(circuit.num_qubits)[num_ancillas:], 2) | ||
corrected[i] += statevector_amplitude | ||
statevector = corrected | ||
np.testing.assert_array_almost_equal(statevector.real, reference) | ||
|
||
@data(5, 10, 15) | ||
def test_mcxvchain_dirty_ancilla_cx_count(self, num_ctrl_qubits): |
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.
More tests that can be removed (I think) especially given #13961. Or actually shouldn't we keep the original test with the MCXVChain gate, that was deprecated but not yet removed? (The same comment applies to other changes in this file).
Summary
The circuit library modernization from #13046 introduced some pending deprecations that this PR is moving to deprecations.
reno is still missing.