Skip to content

Fix barrier label position when bits are reversed #13780

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

Merged
merged 4 commits into from
Feb 5, 2025

Conversation

atharva-satpute
Copy link
Contributor

@atharva-satpute atharva-satpute commented Feb 3, 2025

Summary

Fixes: #13609
Changed the way the barrier label is applied to qubits. Previously, we used the index of the qubit in qargs list to decide if a label should be added here. Now, the label is applied to the qubit with the smallest index in _wire_map, ensuring the label is based on the qubit's position in the _wire_map rather than its position in the node.qargs list

Details and comments

Output:

      init ┌───┐      final 
q_0: ──░───┤ H ├──■─────░───
       ░   └───┘┌─┴─┐   ░   
q_1: ──░────────┤ X ├───░───
       ░        └───┘   ░   
      init      ┌───┐ final 
q_1: ──░────────┤ X ├───░───
       ░   ┌───┐└─┬─┘   ░   
q_0: ──░───┤ H ├──■─────░───
       ░   └───┘        ░   

@atharva-satpute atharva-satpute requested review from nonhermitian and a team as code owners February 3, 2025 14:54
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Feb 3, 2025
@qiskit-bot
Copy link
Collaborator

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 following people are relevant to this code:

@atharva-satpute atharva-satpute marked this pull request as draft February 3, 2025 15:32
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.

This looks like the start of a solid change. The tests are showing trouble in barriers that aren't full width (e.g. the top of the barrier isn't at the top of the drawing), so I think we might need some logic a bit more along the lines of

top_qubit = min(node.qargs, key=self._wire_map.get)
for qubit in node.qargs:
    if qubit in self.qubits:
        label = op.label if qubit == top_qubits else ""
        # ...

(completely untested) to handle those cases.

Please could you also add a test case that's a regression test of what you're fixing, and a bugfix release note?

@jakelishman jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: visualization qiskit.visualization labels Feb 3, 2025
@atharva-satpute
Copy link
Contributor Author

This looks like the start of a solid change. The tests are showing trouble in barriers that aren't full width (e.g. the top of the barrier isn't at the top of the drawing), so I think we might need some logic a bit more along the lines of

top_qubit = min(node.qargs, key=self._wire_map.get)
for qubit in node.qargs:
    if qubit in self.qubits:
        label = op.label if qubit == top_qubits else ""
        # ...

(completely untested) to handle those cases.

Please could you also add a test case that's a regression test of what you're fixing, and a bugfix release note?

I’ve set the default value to float("inf") for cases where the qubit isn’t found in self._wire_map, which can happen when idle_wires = False

@coveralls
Copy link

coveralls commented Feb 4, 2025

Pull Request Test Coverage Report for Build 13157633583

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

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 243 unchanged lines in 19 files lost coverage.
  • Overall coverage decreased (-0.1%) to 88.657%

Files with Coverage Reduction New Missed Lines %
crates/circuit/src/dag_circuit.rs 1 89.36%
qiskit/providers/fake_provider/fake_backend.py 1 85.51%
qiskit/providers/basic_provider/basic_provider_tools.py 1 83.33%
crates/circuit/src/operations.rs 1 89.61%
qiskit/pulse/transforms/canonicalization.py 1 93.44%
qiskit/pulse/transforms/base_transforms.py 2 86.96%
qiskit/pulse/schedule.py 3 88.6%
qiskit/result/result.py 3 83.74%
qiskit/providers/models/backendconfiguration.py 3 85.76%
qiskit/circuit/library/blueprintcircuit.py 3 96.52%
Totals Coverage Status
Change from base Build 13115148220: -0.1%
Covered Lines: 78953
Relevant Lines: 89054

💛 - Coveralls

@atharva-satpute atharva-satpute marked this pull request as ready for review February 4, 2025 15:11
@qiskit-bot
Copy link
Collaborator

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 following people are relevant to this code:

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.

Ace, thank you!

@jakelishman jakelishman modified the milestones: 1.4.0, 1.3.3 Feb 5, 2025
@jakelishman jakelishman enabled auto-merge February 5, 2025 12:44
@jakelishman jakelishman added this pull request to the merge queue Feb 5, 2025
Merged via the queue into Qiskit:main with commit 1ef0d79 Feb 5, 2025
17 checks passed
mergify bot pushed a commit that referenced this pull request Feb 5, 2025
* qa: Fix barrier label position when bits are reversed

* Consider qubit with min index as top qubit

* Fix lint issues

* Fix typo

---------

Co-authored-by: Jake Lishman <jake@binhbar.com>
(cherry picked from commit 1ef0d79)
github-merge-queue bot pushed a commit that referenced this pull request Feb 5, 2025
* qa: Fix barrier label position when bits are reversed

* Consider qubit with min index as top qubit

* Fix lint issues

* Fix typo

---------

Co-authored-by: Jake Lishman <jake@binhbar.com>
(cherry picked from commit 1ef0d79)

Co-authored-by: atharva-satpute <55058959+atharva-satpute@users.noreply.github.com>
@johnbhurst
Copy link

Hmm. Using Qiskit 1.4.1, and the sample code I put in the original ticket 13609:

from qiskit.circuit import QuantumCircuit, QuantumRegister

q = [q0, q1] = QuantumRegister(2, 'q')
circuit = QuantumCircuit(q)
circuit.barrier(label='init')
circuit.h(q0)
circuit.cx(q0, q1)
circuit.barrier(label='final')
circuit.draw(output='mpl', filename='circuit.png')
circuit.draw(output='mpl', filename='circuit_reverse_bits.png', reverse_bits=True)

I am still getting the same incorrect barrier label positions as shown in the images in the original ticket.

Am I missing something?

@atharva-satpute
Copy link
Contributor Author

I missed adding changes for output="mpl" and others. I'll submit a new PR soon

@atharva-satpute
Copy link
Contributor Author

Created #13971

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: visualization qiskit.visualization stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Barrier labels placed wrongly with draw(reverse_bits=True)
5 participants