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

Consider unit in Delay comparisons #13816

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

jakelishman
Copy link
Member

The unit field was previously ignored, allowing delays in dt units to compare equal to those in s. This commit does not add additional on-the-fly unit conversion to the comparison: if the user specified durations in different time multiples, they may have had a reason to consider them non-equal. This could be revisiting after the new system for duration handling lands (i.e. the new functionality for stretch and other delayed scheduling).

Summary

Details and comments

Fix #13812.

The `unit` field was previously ignored, allowing delays in `dt` units
to compare equal to those in `s`. This commit does not add additional
on-the-fly unit conversion to the comparison: if the user specified
durations in different time multiples, they may have had a reason to
consider them non-equal.  This could be revisiting after the new system
for duration handling lands (i.e. the new functionality for `stretch`
and other delayed scheduling).
@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 labels Feb 10, 2025
@jakelishman jakelishman added this to the 1.3.3 milestone Feb 10, 2025
@jakelishman jakelishman requested a review from a team as a code owner February 10, 2025 12:00
@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 13240340536

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 15 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 88.314%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 92.48%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 13239469158: -0.01%
Covered Lines: 78780
Relevant Lines: 89204

💛 - Coveralls

@kevinhartman kevinhartman self-assigned this Feb 10, 2025
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.

LGTM.

@kevinhartman kevinhartman added this pull request to the merge queue Feb 11, 2025
Merged via the queue into Qiskit:main with commit 32eae98 Feb 11, 2025
17 checks passed
@jakelishman jakelishman deleted the delay-py-compare-unit branch February 11, 2025 19:48
mergify bot pushed a commit that referenced this pull request Feb 11, 2025
The `unit` field was previously ignored, allowing delays in `dt` units
to compare equal to those in `s`. This commit does not add additional
on-the-fly unit conversion to the comparison: if the user specified
durations in different time multiples, they may have had a reason to
consider them non-equal.  This could be revisiting after the new system
for duration handling lands (i.e. the new functionality for `stretch`
and other delayed scheduling).

(cherry picked from commit 32eae98)
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2025
The `unit` field was previously ignored, allowing delays in `dt` units
to compare equal to those in `s`. This commit does not add additional
on-the-fly unit conversion to the comparison: if the user specified
durations in different time multiples, they may have had a reason to
consider them non-equal.  This could be revisiting after the new system
for duration handling lands (i.e. the new functionality for `stretch`
and other delayed scheduling).

(cherry picked from commit 32eae98)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
@kevinhartman
Copy link
Contributor

kevinhartman commented Feb 15, 2025

It's probably also worth noting here that the PackedInstruction version of StandardInstruction::Delay should already compare in the same way as what you've done here.

@jakelishman
Copy link
Member Author

Kevin: it does - the discrepancy between it and the Python-space version is what prompted #13812.

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 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.

Delay.__eq__ does not compare the delay unit
4 participants