"cregbundle set to False" warning should not raised when no classical bits are involved #8689

1ucian0 · 2022-09-06T10:20:40Z

@Guillermo-Mijares-Vilarino discovered that the following code raises a very confusing warning:

from qiskit.circuit.library import QFT
qft = QFT(3)
qft.decompose().draw('mpl', reverse_bits=True)

RuntimeWarning: Cregbundle set to False since either reverse_bits or wire_order has been set.
  wire_order=wire_order,

This fix checks if classical bits are involved before touching cregbundle.

Fixes #8690

… bits are involved

qiskit-bot · 2022-09-06T10:20:43Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@enavarro51

coveralls · 2022-09-06T10:53:11Z

Pull Request Test Coverage Report for Build 3162371682

12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.004%) to 84.663%

Totals
Change from base Build 3162371342:	0.004%
Covered Lines:	61510
Relevant Lines:	72653

💛 - Coveralls

Guillermo-Mijares-Vilarino · 2022-09-06T12:30:27Z

This PR would be very helpful for the explanation section I'm adding in #8685.

jakelishman · 2022-09-06T16:44:45Z

Would you like to make this part of a slightly larger issue surrounding the cregbundle parameter? I've just made #8690 to describe it, although the behaviour has been present for a super long time. Your fix here is good with or without touching that issue, I'm just bringing it up since you're touching the code anyway.

jakelishman · 2022-09-06T16:46:18Z

releasenotes/notes/no_warning_with_reverse_bits-b47cb1e357201593.yaml

+---
+fixes:
+  - |
+    When :func:`~circuit_drawer` was used with `reverse_bits=True` on a circuit without classical bits, warning about `cregbundle` was raised, creating confusion. The warning was removed for this case.


This line could do with wrapping - when reno generates the rst for the metapackage, this goes in at a few levels of nesting that might cause its linter to complain.

(It wouldn't hurt for us to get a checker running on Terra to enforce that.)

Fixed in 56d3e38

jakelishman · 2022-09-06T16:49:41Z

test/python/visualization/test_circuit_drawer.py

+    def test_reverse_bits(self):
+        """Test reverse_bits should not raise warnings when no classical qubits:
+        See: https://github.com/Qiskit/qiskit-terra/pull/8689"""
+        circuit = QuantumCircuit(3)
+        circuit.x(1)
+
+        with catch_warnings():
+            simplefilter("error")
+            visualization.circuit_drawer(circuit, reverse_bits=True)


It would be good to test the output of this against the expected one as well, just to be sure.

I think the test suite should automatically fail if the warning is emitted without messing with catch_warnings?

It would be good to test the output of this against the expected one as well, just to be sure.

checking the result in 6a1e223

I think the test suite should automatically fail if the warning is emitted without messing with catch_warnings?

I dont disagree, but I think we have some warnings around. Maybe for another PR?

I removed the warning missing

1ucian0 · 2022-09-18T14:54:17Z

Would you like to make this part of a slightly larger issue surrounding the cregbundle parameter?

I fixed that one too by changing the way that the default is handled for that parameter.

mtreinish

LGTM, just some small doc fixes inline

qiskit/circuit/quantumcircuit.py

qiskit/visualization/circuit/circuit_visualization.py

releasenotes/notes/no_warning_with_reverse_bits-b47cb1e357201593.yaml

"cregbundle set to False" warning should not raised when no classical…

a8fa267

… bits are involved

1ucian0 requested review from a team, nonhermitian and nkanazawa1989 as code owners September 6, 2022 10:20

add link to the issue/PR

9750863

Guillermo-Mijares-Vilarino previously approved these changes Sep 6, 2022

View reviewed changes

jakelishman reviewed Sep 6, 2022

View reviewed changes

1ucian0 added this to the 0.22 milestone Sep 13, 2022

conflict solving

24f7215

1ucian0 dismissed Guillermo-Mijares-Vilarino’s stale review via 24f7215 September 13, 2022 08:00

1ucian0 added 5 commits September 15, 2022 22:30

Merge branch 'main' of github.com:Qiskit/qiskit-terra into fix/8685

9138bae

wrapping

56d3e38

compare result

6a1e223

test resull

92d8e0b

handle default in a consist way

4745408

1ucian0 added 5 commits September 18, 2022 16:58

comparison

67a6c59

remove warning

52be0c7

set default mpl and latex

dc3b4c9

Merge branch 'main' of github.com:Qiskit/qiskit-terra into fix/8685

e35fe5a

adjust test

7bffe06

mtreinish reviewed Sep 30, 2022

View reviewed changes

qiskit/circuit/quantumcircuit.py Outdated Show resolved Hide resolved

qiskit/visualization/circuit/circuit_visualization.py Outdated Show resolved Hide resolved

releasenotes/notes/no_warning_with_reverse_bits-b47cb1e357201593.yaml Outdated Show resolved Hide resolved

Apply suggestions from code review

f95e859

mtreinish added Changelog: Bugfix Include in the "Fixed" section of the changelog automerge labels Sep 30, 2022

kdk approved these changes Sep 30, 2022

View reviewed changes

Merge branch 'main' into fix/8685

5c4b5bc

mergify bot merged commit 2b8a9e0 into Qiskit:main Oct 1, 2022

1ucian0 deleted the fix/8685 branch October 4, 2022 09:51

This was referenced Nov 15, 2022

Simple dynamic circuits raise RuntimeWarning #9129

Closed

Infer cregbundle for all drawers #9133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"cregbundle set to False" warning should not raised when no classical bits are involved #8689

"cregbundle set to False" warning should not raised when no classical bits are involved #8689

1ucian0 commented Sep 6, 2022 •

edited

Loading

qiskit-bot commented Sep 6, 2022

coveralls commented Sep 6, 2022 •

edited

Loading

Guillermo-Mijares-Vilarino commented Sep 6, 2022

jakelishman commented Sep 6, 2022

jakelishman Sep 6, 2022

1ucian0 Sep 15, 2022

jakelishman Sep 6, 2022

1ucian0 Sep 15, 2022

1ucian0 Sep 18, 2022

1ucian0 commented Sep 18, 2022

mtreinish left a comment

"cregbundle set to False" warning should not raised when no classical bits are involved #8689

"cregbundle set to False" warning should not raised when no classical bits are involved #8689

Conversation

1ucian0 commented Sep 6, 2022 • edited Loading

qiskit-bot commented Sep 6, 2022

coveralls commented Sep 6, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3162371682

💛 - Coveralls

Guillermo-Mijares-Vilarino commented Sep 6, 2022

jakelishman commented Sep 6, 2022

jakelishman Sep 6, 2022

Choose a reason for hiding this comment

1ucian0 Sep 15, 2022

Choose a reason for hiding this comment

jakelishman Sep 6, 2022

Choose a reason for hiding this comment

1ucian0 Sep 15, 2022

Choose a reason for hiding this comment

1ucian0 Sep 18, 2022

Choose a reason for hiding this comment

1ucian0 commented Sep 18, 2022

mtreinish left a comment

Choose a reason for hiding this comment

1ucian0 commented Sep 6, 2022 •

edited

Loading

coveralls commented Sep 6, 2022 •

edited

Loading