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

Round durations in GenericBackendV2 #11780

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Feb 13, 2024

Summary

This PR makes sure that the conversion of GenericBackendV2 instruction durations to dt is exact to avoid user warnings during scheduling of type:

UserWarning: Duration is rounded to 616 [dt] = 1.367520e-07 [s] from 1.366887e-07 [s]

Given that the durations are sampled randomly, and the rounded duration is the one used in the scheduling passes, we might as well make sure in advance that the conversion from seconds to dt will be exact and doesn't raise warnings.

Details and comments

I am not sure this qualifies as a bugfix but I think it improves the readability of the test logs. For example, for test_scheduling_backend_v2 in test/python/compiler/test_transpiler.py. Before:

/Users/ept/qiskit_workspace/qiskit/qiskit/circuit/duration.py:37: UserWarning: Duration is rounded to 986 [dt] = 2.188920e-07 [s] from 2.189841e-07 [s]
  warnings.warn(
/Users/ept/qiskit_workspace/qiskit/qiskit/circuit/duration.py:37: UserWarning: Duration is rounded to 2740 [dt] = 6.082800e-07 [s] from 6.083383e-07 [s]
  warnings.warn(
/Users/ept/qiskit_workspace/qiskit/qiskit/circuit/duration.py:37: UserWarning: Duration is rounded to 2697 [dt] = 5.987340e-07 [s] from 5.988312e-07 [s]
  warnings.warn(
/Users/ept/qiskit_workspace/qiskit/qiskit/circuit/duration.py:37: UserWarning: Duration is rounded to 178 [dt] = 3.951600e-08 [s] from 3.956636e-08 [s]
  warnings.warn(
.
----------------------------------------------------------------------
Ran 1 test in 0.548s

OK

After:

.
----------------------------------------------------------------------
Ran 1 test in 0.506s

OK

@ElePT ElePT added the Changelog: None Do not include in changelog label Feb 13, 2024
@ElePT ElePT added this to the 1.0.0 milestone Feb 13, 2024
@ElePT ElePT requested review from jyu00 and a team as code owners February 13, 2024 10:23
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Feb 13, 2024
@coveralls
Copy link

coveralls commented Feb 13, 2024

Pull Request Test Coverage Report for Build 8392874457

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 89.321%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 91.86%
Totals Coverage Status
Change from base Build 8391327735: 0.02%
Covered Lines: 59794
Relevant Lines: 66943

💛 - Coveralls

@ElePT ElePT mentioned this pull request Feb 13, 2024
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 feels sufficiently non-critical to me that my vote would be to merge it in 1.0.1 rather than post-rc, but I don't feel overly strongly.

Comment on lines 420 to 423
if duration is not None:
# Ensure exact conversion of duration from seconds to dt
dt = _QUBIT_PROPERTIES["dt"]
duration = round(duration / dt) * dt
Copy link
Member

Choose a reason for hiding this comment

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

Technically this can cause the duration to fall outside the distribution it was drawn from. I don't know how much you care about that particular edge case, but strictly it's possible. Consider drawing from the uniform distribution [0.09, 0.15) with dt=0.04 - value arbitrarily close to 0.09 and 0.15 will be rounded to 0.08 and 0.16 respectively, outside the range.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ElePT perhaps you can clamp the result to the lower and upper bounds here, and ensure those bounds are a multiple of dt when declaring the constants for the ranges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion @kevinhartman, I applied it in 0bb2fcd.

@jakelishman jakelishman modified the milestones: 1.0.0, 1.0.1 Feb 14, 2024
@ElePT ElePT reopened this Feb 16, 2024
@jakelishman jakelishman modified the milestones: 1.0.1, 1.0.2 Feb 22, 2024
@kevinhartman kevinhartman modified the milestones: 1.0.2, 1.0.3 Mar 7, 2024
Copy link
Contributor

@kevinhartman kevinhartman 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 good to me. Thanks for the changes! 😄

@kevinhartman kevinhartman added this pull request to the merge queue Mar 25, 2024
Merged via the queue into Qiskit:main with commit 35a8e53 Mar 25, 2024
12 checks passed
mergify bot pushed a commit that referenced this pull request Mar 25, 2024
* Round durations

* Update generic_backend_v2.py

* Clamp rounded durations

(cherry picked from commit 35a8e53)
github-merge-queue bot pushed a commit that referenced this pull request Mar 28, 2024
* Round durations

* Update generic_backend_v2.py

* Clamp rounded durations

(cherry picked from commit 35a8e53)

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
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 stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants