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

Extended the from_backend method of InstructionDurations to support both BackendV1 and BackendV2 #12941

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

shravanpatel30
Copy link
Contributor

Summary

This PR fixes #12760

I have extended the from_backend method of InstructionDurations so that it supports both BackendV1 and BackendV2 (also GenericBackendV2). To get the instruction durations from the BackendV2, I have used the attribute target. Also, in GenericBackendV2, the (duration, error) for delay and reset is set to (None, None) and this None value raises an error in the update() method of InstructionDurations. So, I am skipping the instructions which have None as the duration (let me know if this is not what we want).

@1ucian0, please review this and let me know if this is acceptable. If you want I can also add tests and/or new release notes for the changes I have made.

Thanks

Details and comments

@shravanpatel30 shravanpatel30 requested a review from a team as a code owner August 11, 2024 21:35
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 11, 2024
@qiskit-bot
Copy link
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 following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Aug 11, 2024

Pull Request Test Coverage Report for Build 10428323140

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 449 unchanged lines in 23 files lost coverage.
  • Overall coverage decreased (-0.2%) to 89.558%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 90.69%
qiskit/providers/fake_provider/fake_pulse_backend.py 1 95.24%
qiskit/synthesis/unitary/qsd.py 1 97.58%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.39%
qiskit/providers/fake_provider/fake_qasm_backend.py 2 95.65%
qiskit/compiler/transpiler.py 3 92.39%
qiskit/transpiler/passmanager_config.py 3 95.95%
qiskit/transpiler/passes/scheduling/padding/dynamical_decoupling.py 4 94.86%
qiskit/transpiler/preset_passmanagers/common.py 5 89.5%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 6 90.84%
Totals Coverage Status
Change from base Build 10339507099: -0.2%
Covered Lines: 67577
Relevant Lines: 75456

💛 - Coveralls

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! However I think there's a simpler way of solving this, see the comments below 🙂 Also, yes, this would need a releasenote (under fixes) and a test (for which you could use the example from #12760)

@@ -62,7 +63,7 @@ def __str__(self):
return string

@classmethod
def from_backend(cls, backend: Backend):
def from_backend(cls, backend: Backend | BackendV2):
Copy link
Contributor

Choose a reason for hiding this comment

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

Backend includes BackendV2 so we don't need an extra type here 🙂

@@ -62,7 +63,7 @@ def __str__(self):
return string

@classmethod
def from_backend(cls, backend: Backend):
def from_backend(cls, backend: Backend | BackendV2):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could just solve this as

def from_backend(cls, backend):
    if isinstance(backend, BackendV2):
        return backend.target.durations()
        
    # old code goes here as it is

@shravanpatel30
Copy link
Contributor Author

Hi @Cryoris, thanks for reviewing the PR. I have incorporated the changes that you requested. Let me know if I need to make any more edits.

Thanks

---
fixes:
- |
The `from_backend` method of :class:`.InstructionDurations` now allows extracting instruction durations from both `BackendV1` and `BackendV2`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `from_backend` method of :class:`.InstructionDurations` now allows extracting instruction durations from both `BackendV1` and `BackendV2`.
Fixed a bug where :meth:`.InstructionDurations.from_backend` did not work for :class:`.BackendV2` backends.
Fixed `#12760 <https://github.com/Qiskit/qiskit/issues/12760>`__.

@Cryoris Cryoris added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Aug 16, 2024
@shravanpatel30
Copy link
Contributor Author

Hi @Cryoris, I have edited the releasenote as per your comment.

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.

The fix looks good to me, and it looks like you applied all of @Cryoris's suggestions, thanks @shravanpatel30! We can probably backport it to 1.2 too, I'll add the stable backport label.

@ElePT ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Aug 19, 2024
@ElePT ElePT added this pull request to the merge queue Aug 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 19, 2024
@ElePT ElePT added this pull request to the merge queue Aug 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 19, 2024
@shravanpatel30
Copy link
Contributor Author

Hi @ElePT, I received an email that this pull request was removed from merge queue. Do I need to make any changes to this pull request?

Thanks

@ElePT ElePT added this pull request to the merge queue Aug 21, 2024
@ElePT
Copy link
Contributor

ElePT commented Aug 21, 2024

This was probably due to unrelated status check issues, I'll re-queue and keep an eye on the PR. No action needed @shravanpatel30 :)

Merged via the queue into Qiskit:main with commit 6107799 Aug 21, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Aug 21, 2024
…rt both `BackendV1` and `BackendV2` (#12941)

* Extended the `from_backend` method of `InstructionDurations` to support `GenericBackendV2`

* Simplified the `from_backend` method to allow using `BackendV2`. Added a test and a releasenote.

* Made changes to the releasenote.

(cherry picked from commit 6107799)
github-merge-queue bot pushed a commit that referenced this pull request Aug 21, 2024
…rt both `BackendV1` and `BackendV2` (#12941) (#13009)

* Extended the `from_backend` method of `InstructionDurations` to support `GenericBackendV2`

* Simplified the `from_backend` method to allow using `BackendV2`. Added a test and a releasenote.

* Made changes to the releasenote.

(cherry picked from commit 6107799)

Co-authored-by: Shravan Patel <78003234+shravanpatel30@users.noreply.github.com>
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 Community PR PRs from contributors that are not 'members' of the Qiskit repo stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

InstructionDurations.from_backend(...) does not support BackendV2
5 participants