Skip to content
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

The parameter idle_wires default flipped to False in all circuit drawers #13865

Merged
merged 20 commits into from
Mar 6, 2025

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Feb 17, 2025

Summary

Closes #12361

Details and comments

As QPUs are getting bigger, also the circuits are growing in terms of qubits.

@1ucian0 1ucian0 requested review from nonhermitian and a team as code owners February 17, 2025 22:16
@qiskit-bot
Copy link
Collaborator

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

@coveralls
Copy link

coveralls commented Feb 18, 2025

Pull Request Test Coverage Report for Build 13696173867

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.

Details

  • 7 of 8 (87.5%) changed or added relevant lines in 2 files are covered.
  • 1892 unchanged lines in 80 files lost coverage.
  • Overall coverage increased (+1.3%) to 88.501%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/circuit/circuit_visualization.py 4 5 80.0%
Files with Coverage Reduction New Missed Lines %
crates/circuit/src/operations.rs 1 90.37%
qiskit/circuit/_classical_resource_map.py 1 76.62%
qiskit/circuit/commutation_checker.py 1 94.12%
qiskit/circuit/controlflow/_builder_utils.py 1 96.97%
qiskit/circuit/parameter.py 1 96.72%
qiskit/compiler/transpiler.py 1 98.78%
qiskit/primitives/utils.py 1 92.86%
qiskit/result/distributions/quasi.py 1 95.92%
qiskit/synthesis/evolution/product_formula.py 1 89.8%
qiskit/transpiler/passes/layout/apply_layout.py 1 96.67%
Totals Coverage Status
Change from base Build 13635545005: 1.3%
Covered Lines: 71838
Relevant Lines: 81172

💛 - Coveralls

@1ucian0 1ucian0 added this to the 2.0.0 milestone Feb 24, 2025
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say that this is a better default, but then I thought: actually I use the drawers quite a lot for small-scale drawing, and I think suppressing idle wires there by default is quite possibly surprising behaviour.

I wonder if it's worth having idle_wires take a number to use as a cut-off size: if there are n or more idle wires (possibly only count qubit ones), hide them, and if there's fewer, show them? So idle_wires=0 is the same as the current idle_wires=False. We'd have to catch idle_wires=True and turn it into idle_wires=math.inf, potentially.

Alternatively, we ignore the int stuff, and add a third option to idle_wires: "layout". I think this might be the happiest middle-ground: if the circuit has been transpiled to a layout (so the left margin will indicate qubit mapping), then we set idle_wires=False, and if it hasn't, we set idle_wires=True. (Could also call the option "auto" or something.)

I'm not 100% convinced of that, I'm just slightly concerned about breaking usability of drawing small-scale circuits - I would think there's a pretty sizeable number of people who almost invariably draw those, and it'd be confusing if wires are sometimes hidden on them.

Comment on lines 2 to 4
upgrade_visualization:
- |
The default for the parameter ``idle_wires`` changed to ``False`` in all the circuit drawers. If you still want to see wires without instructions, set ``idle_wires=True`` explicitly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might help to either explain a bit more, or put in a couple of text-mode drawings just to illustrate the difference? I'm not sure users necessarily know what the option is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. done in 4f83dc7

@@ -1117,7 +1117,9 @@ def test_idle_wires_barrier(self):
circuit.barrier()

fname = "idle_wires_barrier.png"
self.circuit_drawer(circuit, output="mpl", cregbundle=False, filename=fname)
self.circuit_drawer(
circuit, output="mpl", cregbundle=False, filename=fname, idle_wires=False
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally found that a test with the description "Test that idle_wires False works with barrier" was not doing idle_wires=False

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Luciano, this does feel like a good default now.

Comment on lines 389 to 391
def _get_layered_instructions(
circuit, reverse_bits=False, justify=None, idle_wires=True, wire_order=None, wire_map=None
circuit, reverse_bits=False, justify=None, idle_wires="auto", wire_order=None, wire_map=None
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave this as idle_wires: bool, and resolve the "auto" at the top level of the drawers?

Comment on lines 208 to 210
default_idle_wires = config.get("circuit_idle_wires", False)
default_idle_wires = config.get("circuit_idle_wires", "auto")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we now resolve "auto" into either True or False immediately after line 227, so the rest of the visualisers don't need to change?

Comment on lines 3195 to 3200
idle_wires: Include (or not) idle wires (wires with no circuit elements)
in output visualization. The string ``"auto"`` is also possible, in which
case idle wires are show except that the circuit has a layout attached.
Default is ``"auto"`` unless the
user config file (usually ``~/.qiskit/settings.conf``) has an
alternative value set. For example, ``circuit_idle_wires = True``.
alternative value set. For example, ``circuit_idle_wires = False``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the configuration-file logic to make it possible to specify "auto" in there too?

1ucian0 and others added 3 commits March 4, 2025 17:32
there's some bloch sphere plot failure which likely shouldn't be related to this, let's see if it persists
handle "auto" at the circuit_drawer level and subsequently use on booleans in internal logic
@jakelishman jakelishman self-assigned this Mar 6, 2025
Co-authored-by: Jake Lishman <jake@binhbar.com>
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this locally and it seems to do what I expect - thanks for the changes to the default, Luciano, and for getting it over the line, Julien.

@jakelishman jakelishman enabled auto-merge March 6, 2025 11:07
@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog Changelog: API Change Include in the "Changed" section of the changelog mod: visualization qiskit.visualization labels Mar 6, 2025
@jakelishman jakelishman added this pull request to the merge queue Mar 6, 2025
Merged via the queue into Qiskit:main with commit 2ec8f8f Mar 6, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: New Feature Include in the "Added" section of the changelog mod: visualization qiskit.visualization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default to idle_wires=False in drawers
5 participants