Skip to content

Fix dt units in test util function convert_to_target#8000

Merged
mergify[bot] merged 3 commits into
Qiskit:mainfrom
kevinhartman:fix/dt-target-convert
Apr 29, 2022
Merged

Fix dt units in test util function convert_to_target#8000
mergify[bot] merged 3 commits into
Qiskit:mainfrom
kevinhartman:fix/dt-target-convert

Conversation

@kevinhartman
Copy link
Copy Markdown
Contributor

Summary

Previously, convert_to_target was reading dt from the configuration/conf file, expecting it to be in seconds, when it is actually in nanoseconds.

The effect of this was that FakeBackendV2 targets would return a InstructionDurations with all durations as 0, since all durations round to 0 when divided by the erroneously large dt and converted to an integer.

@kevinhartman kevinhartman added type: qa Issues and PRs that relate to testing and code quality stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. labels Apr 29, 2022
@kevinhartman kevinhartman requested review from a team and jyu00 as code owners April 29, 2022 20:34
@qiskit-bot
Copy link
Copy Markdown
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:

  • @Qiskit/terra-core

@kevinhartman kevinhartman requested a review from mtreinish April 29, 2022 20:35
@mtreinish mtreinish added this to the 0.20.2 milestone Apr 29, 2022
Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Good catch, this lgtm. Just one comment on the release note and a suggestion on the unit test.

Comment thread test/python/providers/test_fake_backends.py
Comment thread releasenotes/notes/fix-target-dt-4d306f1e9b07f819.yaml Outdated
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 2247717376

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.005%) to 84.323%

Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 2236086132: -0.005%
Covered Lines: 54518
Relevant Lines: 64654

💛 - Coveralls

@mergify mergify Bot merged commit ce4f94d into Qiskit:main Apr 29, 2022
mergify Bot pushed a commit that referenced this pull request Apr 29, 2022
* Multilpy dt value by conversion factor when reading from conf file.

* Add simple test and release note.

* Update releasenotes/notes/fix-target-dt-4d306f1e9b07f819.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
(cherry picked from commit ce4f94d)
@HuangJunye
Copy link
Copy Markdown
Collaborator

@kevinhartman Good catch. I thought I corrected the error when I made the FakeBackendV2 PR #7643 but actually introduced another error...

a814fa7#diff-eb1c104ba5abb83f1ab8c8b159c2b5537b234aca17a448fa15cbc55d0461ece7L85-R109

qiskit-ibm-runtime has its own copy of backend_converter.py. It should be updated too. CC @rathishcholarajan

mergify Bot added a commit that referenced this pull request May 2, 2022
* Multilpy dt value by conversion factor when reading from conf file.

* Add simple test and release note.

* Update releasenotes/notes/fix-target-dt-4d306f1e9b07f819.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
(cherry picked from commit ce4f94d)

Co-authored-by: Kevin Hartman <kevin@hart.mn>
ElePT pushed a commit to ElePT/qiskit-ibm-provider that referenced this pull request Oct 9, 2023
…it#8000)

* Multilpy dt value by conversion factor when reading from conf file.

* Add simple test and release note.

* Update releasenotes/notes/fix-target-dt-4d306f1e9b07f819.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. type: qa Issues and PRs that relate to testing and code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants