Deprecate the condition attribute and related functionality by mtreinish · Pull Request #13223 · Qiskit/qiskit

mtreinish · 2024-09-25T22:03:38Z

Summary

This commit deprecates the Instruction.condition and c_if() method for removal in 2.0. This functionality has been superseded by the more sophisiticated IfElseOp for some time now. Removing the condition property will simplify the Qiskit data model as it is implemented in a kind of ad-hoc manner that adds additional overhead to manually check it in many places.

For the unittest modifications the deprecation warning for the .condtion getter is suppressed for the entire suite because this gets triggered internally by the transpiler and a lot of other places, including from rust code as until it is removed we need to use it to check that piece of the data model.

Details and comments

Fixes #9556

This commit deprecates the Instruction.condition and c_if() method for removal in 2.0. This functionality has been superseded by the more sophisiticated `IfElseOp` for some time now. Removing the condition property will simplify the Qiskit data model as it is implemented in a kind of ad-hoc manner that adds additional overhead to manually check it in many places. For the unittest modifications the deprecation warning for the .condtion getter is suppressed for the entire suite because this gets triggered internally by the transpiler and a lot of other places, including from rust code as until it is removed we need to use it to check that piece of the data model. Fixes Qiskit#9556

qiskit-bot · 2024-09-25T22:03:44Z

One or more of the following people are relevant to this code:

@enavarro51
@Cryoris
@Qiskit/terra-core
@ajavadia
@levbishop
@mtreinish
@nkanazawa1989
@t-imamichi

coveralls · 2024-09-26T11:35:19Z

Pull Request Test Coverage Report for Build 11599100254

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

81 of 83 (97.59%) changed or added relevant lines in 29 files are covered.
40 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.02%) to 88.73%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/qpy/binary_io/circuits.py	4	6	66.67%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	1	92.2%
crates/qasm2/src/lex.rs	2	93.23%
crates/qasm2/src/parse.rs	6	97.62%
qiskit/result/mitigation/utils.py	8	89.74%
qiskit/result/mitigation/correlated_readout_mitigator.py	10	89.69%
qiskit/result/mitigation/local_readout_mitigator.py	13	90.15%

Totals
Change from base Build 11598008270:	0.02%
Covered Lines:	75638
Relevant Lines:	85245

💛 - Coveralls

ElePT

Wow, this PR is long. I didn't get to review all tests but I left a few minor suggestions that I saw in the source code/reno.

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

ElePT

LGTM! I just found a couple of deprecation warning assertions that I think would be good to clarify (maybe adding a comment explaining how they relate to this deprecation like you did in other cases).

ElePT · 2024-09-27T13:20:11Z

+        with self.assertWarns(DeprecationWarning):
+            self.assertTrue(Operator(circuit).equiv(Operator(result)))


Where is the warning triggered here?

Operator(circuit) runs circuit.to_instruction() which checks for condition iirc.

ElePT · 2024-09-27T13:22:14Z

+        with self.assertWarns(DeprecationWarning):
+            new_qc = ResetAfterMeasureSimplification()(qc)


Do you think this class raising a deprecation warning is a desired outcome (I'm not sure why it's raised)? Maybe the stack level of the warning isn't correct?

The testing harness will trigger if the warning is raised at any stack level even if it's not user code. This is to let us catch internal usage of deprecated functionality in our dependencies. In this case the ResetAfterMeasureSimplification pass only works with c_if and condition. so the warning is coming from inside the pass.

This does raise a good question about what we want to do with that pass, we probably should convert it to an if statement internally. I don't remember what I was thinking when I opened this 2 weeks ago, probably that I would do it in a followup PR. But it would be better if I do it here instead.

When I went to do this I remember why I didn't change it. I decided at the time that leaving the pass using c_if for 1.x made the most sense as using an IfElseOp instead could have been viewed as a potential change in behavior and wanted to save that for 2.0.

I agree with not changing the behavior yet. I brought this up thinking about downstream packages and how they sometimes get "flooded" by our deprecation warnings even if they aren't actionable. But I guess it's on them to set the stack level they want to see in their tests.

This commit fixes some places in the code where we were using the deprecated functionality where it needed a path to persist the behavior or where the deprecation warning became a performance bottleneck. The code is updated accordingly and to catch issues like this in the future the warning filters are adjusted to be more selective.

ElePT

LGTM

ElePT · 2024-10-31T13:43:02Z

+        with self.assertWarns(DeprecationWarning):
+            new_qc = ResetAfterMeasureSimplification()(qc)


I agree with not changing the behavior yet. I brought this up thinking about downstream packages and how they sometimes get "flooded" by our deprecation warnings even if they aren't actionable. But I guess it's on them to set the stack level they want to see in their tests.

wshanks · 2024-11-05T20:39:47Z

+        qc = QuantumCircuit(2, 2)
+        qc.h(0)
+        qc.x(0).c_if(0, 1)
+        qc.z(1.c_if(1, 0)
+        qc.measure(0, 0)
+        qc.measure(1, 1)
+
+    can be rewritten as::
+
+        qc = QuantumCircuit(2, 2)
+        qc.h(0)
+        with expected.if_test((expected.clbits[0], True)):
+            qc.x(0)
+        with expected.if_test((expected.clbits[1], False)):
+            qc.z(1)
+        qc.measure(0, 0)
+        qc.measure(1, 1)


I just started addressing this deprecation warning and noticed some issues here. I think it should be qc.z(1) (missing parenthesis). Also, I think expected should be qc?

mtreinish added priority: high Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog. mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Sep 25, 2024

mtreinish added this to the 1.3.0 milestone Sep 25, 2024

mtreinish requested review from a team, jyu00 and nonhermitian as code owners September 25, 2024 22:03

mtreinish added 2 commits September 26, 2024 07:10

Add missing assertWarns

3669504

Merge branch 'main' into deprecate-condition_cif

0ce9f3f

Handle deprecation warnings in visual tests too

ecd2976

ElePT reviewed Sep 26, 2024

View reviewed changes

Comment thread qiskit/circuit/instruction.py Outdated

Comment thread releasenotes/notes/deprecate-condition_c_if-9548e5522814fe9c.yaml Outdated

Comment thread releasenotes/notes/deprecate-condition_c_if-9548e5522814fe9c.yaml Outdated

Apply suggestions from code review

576b523

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

ElePT reviewed Sep 27, 2024

View reviewed changes

mtreinish added 3 commits October 7, 2024 15:22

Merge remote-tracking branch 'origin/main' into deprecate-condition_cif

5edf528

Use private attribute in py->rust conversion

d4c868a

mtreinish requested a review from ElePT October 7, 2024 21:26

Add missing test updates

099b610

ElePT self-assigned this Oct 21, 2024

ElePT and others added 4 commits October 28, 2024 10:52

Merge branch 'main' into deprecate-condition_cif

1987fb2

Merge remote-tracking branch 'origin/main' into deprecate-condition_cif

4bf68ea

Add filter for aer's condition usage and dag drawer

706d24d

Fix lint

5a77b3e

ElePT approved these changes Oct 31, 2024

View reviewed changes

ElePT added this pull request to the merge queue Oct 31, 2024

Merged via the queue into Qiskit:main with commit c6efb76 Oct 31, 2024

mtreinish deleted the deprecate-condition_cif branch October 31, 2024 14:56

wshanks reviewed Nov 5, 2024

View reviewed changes

		with self.assertWarns(DeprecationWarning):
		self.assertTrue(Operator(circuit).equiv(Operator(result)))

		with self.assertWarns(DeprecationWarning):
		new_qc = ResetAfterMeasureSimplification()(qc)

Conversation

mtreinish commented Sep 25, 2024

Summary

Details and comments

Uh oh!

qiskit-bot commented Sep 25, 2024

Uh oh!

coveralls commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 11599100254

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

ElePT left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ElePT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElePT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Sep 26, 2024 •

edited

Loading

ElePT left a comment •

edited

Loading