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

Construct subclasses of InstructionDurations from backend #11501

Merged
merged 3 commits into from
Jan 6, 2024

Conversation

dieris
Copy link
Contributor

@dieris dieris commented Jan 5, 2024

Summary

Modify construction of InstructionDurations.from_backend to return an instance of any subclass, as in the use model for dynamic circuits on qiskit-ibm-provider

Details and comments

In the current behavior DynamicInstructionDurations.from_backend() (a subclass of InstructionDurations) returns an instance of InstructionDurations (and corresponding durations) instead.

@dieris dieris requested a review from a team as a code owner January 5, 2024 20:52
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Jan 5, 2024

Pull Request Test Coverage Report for Build 7427911562

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 87.591%

Totals Coverage Status
Change from base Build 7426524855: 0.003%
Covered Lines: 59475
Relevant Lines: 67901

💛 - Coveralls

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 seems straightforwards and safe, thanks. This class doesn't look to have been designed for subclassing (there's no contract for when it will call its own methods, so it's fragile to depend on), but it's a simple mostly-data class, so it's probably not too risky if others are subclassing it.

Can you add a one-line bugfix release note mentioning the change?

@dieris
Copy link
Contributor Author

dieris commented Jan 5, 2024

Actually, using the right constructor now causes an error on qiskit-ibm-provider, but I'll open an issue there 😄

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.

That seems weird, unless InstructionDurations.from_backend is already broken for IBMBackend (which it could well be - I'm pretty sure from_backend assumes a BackendV1 and has never been updated). I'm fine for this PR update the call here, because really, it's pretty rare for constructors to be safe for virtual override - much like __init__ is near-always necessarily overridden by subclasses, I'd expect all other constructors to be as well.

@jakelishman jakelishman added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jan 5, 2024
@jakelishman jakelishman added this to the 1.0.0 milestone Jan 5, 2024
@jakelishman jakelishman added this pull request to the merge queue Jan 6, 2024
Merged via the queue into Qiskit:main with commit fbb0938 Jan 6, 2024
13 checks passed
ShellyGarion pushed a commit to ShellyGarion/qiskit-terra that referenced this pull request Jan 18, 2024
…1501)

* Construct InstructionDuration subclass

* add reno

* Add Sphinx cross-refs

---------

Co-authored-by: Jake Lishman <[email protected]>
@mtreinish
Copy link
Member

@Mergifyio backport stable/0.46

Copy link
Contributor

mergify bot commented Feb 6, 2024

backport stable/0.46

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Feb 6, 2024
* Construct InstructionDuration subclass

* add reno

* Add Sphinx cross-refs

---------

Co-authored-by: Jake Lishman <[email protected]>
(cherry picked from commit fbb0938)
github-merge-queue bot pushed a commit that referenced this pull request Feb 6, 2024
…11727)

* Construct InstructionDuration subclass

* add reno

* Add Sphinx cross-refs

---------

Co-authored-by: Jake Lishman <[email protected]>
(cherry picked from commit fbb0938)

Co-authored-by: Diego Ristè <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants