-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
InstructionDurations from BackendV2 #11528
base: main
Are you sure you want to change the base?
Conversation
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 the following people are requested to review this:
|
elif isinstance(backend, BackendV2): | ||
target = backend.target | ||
inst_with_no_props = {"delay"} | ||
inst_duration = None | ||
for name in target.operation_names: | ||
for qubits, inst_props in target._gate_map[name].items(): | ||
if name in inst_with_no_props: | ||
# Setting the duration for 'delay' to zero. | ||
inst_duration = 0.0 | ||
else: | ||
try: | ||
inst_duration = inst_props.duration | ||
except AttributeError: | ||
logger.info("%s on %s did not report any duration", name, qubits) | ||
continue | ||
instruction_durations.append((name, qubits, inst_duration, "s")) | ||
|
||
try: | ||
dt = target.dt | ||
except AttributeError: | ||
logger.info("Backend Target didn't report any dt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use backend.target.durations()
(https://docs.quantum.ibm.com/api/qiskit/qiskit.transpiler.Target#durations)? It should already have most of this logic present in there so you can make this branch one line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtreinish , Haha! :) I was working on a different PR, I discovered InstructionDurations.from_backend()
didn't have the BackendV2
and did this PR.
A much better way: backend.target.durations()
didn't cross my mind :)
Thanks, for the suggestion :)
I will add the suggestion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtreinish , added suggestion, please see if this is good enough :)
Pull Request Test Coverage Report for Build 8315482005Details
💛 - Coveralls |
Given :code:`dt` and :code:`dtm` for a :class:`.BackendV1` or :class:`.BackendV2` are unequal, | ||
:meth:`~.InstructionDurations.from_backend` does not raise any error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any change in code behavior with this PR? dtm
is acquisition sampling rate and doesn't impact the instruction duration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkanazawa1989 ,
According to,
TranspilerError: If dt and dtm is different in the backend. |
I should get TranspilerError
if dt
!=dtm
, I just checked it, and found it doesn't.
So, I added this line in release note and added test test_works_unequal_dt_dtm
to make sure this case is covered :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The document should be wrong. See p.33 of https://arxiv.org/abs/1809.03452. dtm defines the sampling rate of digitizer, which can be independent of dt that defines the sampling rate of AWG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this upgrade note is not necessary because TranspilerError
was not actually raised (just accidentally documented).
.../notes/Add_support_for_BackendV2_in_instruction_durations_from_backend-9ce722879c870377.yaml
Outdated
Show resolved
Hide resolved
with self.assertRaises(TranspilerError): | ||
durations.get(self.example_gate, self.example_qubit[0]) | ||
|
||
def test_works_unequal_dt_dtm(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this test? InstructionDuration
is the object used for scheduling, and dtm
doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkanazawa1989 , This line was the motivation:
TranspilerError: If dt and dtm is different in the backend. |
Am I missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MozammilQ for updating the test and error handling. Several minor fix would be required but overall this PR is good to go.
Given :code:`dt` and :code:`dtm` for a :class:`.BackendV1` or :class:`.BackendV2` are unequal, | ||
:meth:`~.InstructionDurations.from_backend` does not raise any error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this upgrade note is not necessary because TranspilerError
was not actually raised (just accidentally documented).
@nkanazawa1989 , see if this is good enough :) |
@dieris: this PR is generally needed on Qiskit (for Qiskit 1.1 in 3 months' time), but I suspect might have some effects for work you did recently on |
yes, it looks like we had a similar idea, but the |
Support for :class:`.BackendV2` in :meth:`.InstructionDurations.from_backend` is added. | ||
|
||
Users can have an object of :class:`.InstructionDurations` using :class:`.BackendV2` | ||
from :meth:`.from_backend` with followig code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from :meth:`.from_backend` with followig code. | |
from :meth:`.InstructionDurations.from_backend` with followig code. |
to fix doc error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MozammilQ this looks good to me now, except for the docs generation failure. Can we merge this once all test pass?
@nkanazawa1989 , Please, see if this is good enough :) |
@1ucian0 , |
Summary
This PR adds the support for
BackendV2
in methodfrom_backend
of classInstructionDurations
in
qiskit.transpiler.instruction_durations.py
Credit:
@nkanazawa1989 , for help and ideas :)
Details and comments
Earlier,
InstructionDurations.from_backend()
could support onlyBackendV1
.In this PR, now
BackendV2
can also be passed into methodfrom_backend
.