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

Fix string formatting in pulse drawer #9540

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Fix string formatting in pulse drawer #9540

merged 1 commit into from
Mar 30, 2023

Conversation

Randl
Copy link
Contributor

@Randl Randl commented Feb 7, 2023

Summary

This a small fix to the formatting, which used an expression of form "%s" % time * dt. Since the % has higher priority it end up multiplying string by floating point number dt. I think the intent was to use time * dt for formatting

@Randl Randl requested review from a team and nonhermitian as code owners February 7, 2023 07:56
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Feb 7, 2023
@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 the following people are requested to review this:

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4111690264

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.277%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/pulse/matplotlib.py 0 1 0.0%
Totals Coverage Status
Change from base Build 4107383485: 0.0%
Covered Lines: 67250
Relevant Lines: 78861

💛 - Coveralls

@HuangJunye
Copy link
Collaborator

@Randl Thanks for your contribution. Can you please provide some context for this fix. What is the current behavior and what it is after the fix?

@HuangJunye HuangJunye self-assigned this Feb 7, 2023
@Randl
Copy link
Contributor Author

Randl commented Feb 7, 2023

The following is minimal example:

from qiskit.pulse.channels import DriveChannel
from qiskit.pulse.instructions import ShiftPhase
phase = 10
channel = DriveChannel(0)
ShiftPhase(phase, channel).draw(table=True, channels=[channel], dt=2)

It will crush if dt is non-integer, and otherwise will print repeated t=0 string in the table dt times

@woodsp-ibm
Copy link
Member

>>> "t = %s" % 2 * 5
't = 2t = 2t = 2t = 2t = 2'
>>> "t = %s" % 2 * 5.5
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't multiply sequence by non-int of type 'float'


>>> "t = %s" % (2 * 5)
't = 10'
>>> "t = %s" % (2 * 5.5)
't = 11.0'

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

We need to extend our visual tests for capturing this. In the meanwhile, let's merge it. Thanks!

@1ucian0 1ucian0 added this pull request to the merge queue Mar 30, 2023
@1ucian0 1ucian0 changed the title Fix string formatting Fix string formatting in pulse drawer Mar 30, 2023
@1ucian0 1ucian0 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Mar 30, 2023
Merged via the queue into Qiskit:main with commit 3ab5715 Mar 30, 2023
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Apr 5, 2023
giacomoRanieri pushed a commit to giacomoRanieri/qiskit-terra that referenced this pull request Apr 16, 2023
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants