Skip to content

Conversation

mtreinish
Copy link
Member

Summary

In the recently merged #13783 a bug slipped in an unexercised code path where we were dividing by dt instead of multiplying. This was fixed in the common code path where dt is an int, but nothing in the data model disallows float durations for dt units despite it not being real. Since the data model allowed it the rust code needed to support it and that path did the wrong operation. This commit fixes that oversight in the rust code and adds a test for it.

Details and comments

In the recently merged Qiskit#13783 a bug slipped in an unexercised code path
where we were dividing by dt instead of multiplying. This was fixed in
the common code path where dt is an int, but nothing in the data model
disallows float durations for dt units despite it not being real. Since
the data model allowed it the rust code needed to support it and that
path did the wrong operation. This commit fixes that oversight in the
rust code and adds a test for it.
@mtreinish mtreinish added the Changelog: None Do not include in changelog label Feb 19, 2025
@mtreinish mtreinish added this to the 2.0.0 milestone Feb 19, 2025
@mtreinish mtreinish requested a review from a team as a code owner February 19, 2025 15:30
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 13416288259

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • 23 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 88.086%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/circuit_duration.rs 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.23%
crates/qasm2/src/lex.rs 4 91.98%
crates/qasm2/src/parse.rs 18 96.68%
Totals Coverage Status
Change from base Build 13411473274: -0.02%
Covered Lines: 78335
Relevant Lines: 88930

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Whoops

@ElePT ElePT added this pull request to the merge queue Feb 20, 2025
Merged via the queue into Qiskit:main with commit a2da24b Feb 20, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants