Add qubit_properties to Target class #7736
Conversation
This commit adds a new attribute to the target class for defining qubit properties. This was originally not included in the target because nothing in the transpiler supports using any qubit properties. However, in a future PR I'm planning to modify the default heuristic layout score used by VF2Layout to factor in T1 and/or T2 in addition to the measurement error rate it's using now. To do this though we need to ensure that we have these values available via the target. This commit also is the first time we've had to expand the set of information that can optionally be pecified in a target. I opted to just make a straight addition here rather than versioning it. There should be no backwards compatibility issue for users or provider maintainers on upgrade because not specifying a qubit properties payload will not change any behavior the Target and BackendV2 objects will behave exactly as before. I was thinking we'd only use a version if we needed to make a backwards incompatible change in the future. That way we can support both versions of the class a the same time while we migrated to a newer interface. But for simple additions like this that don't effect the behavior or cause any issues with compatibility we can simply add them to the class.
mtreinish
added
the
Changelog: New Feature
Pull Request Test Coverage Report for Build 2069003425
💛 - Coveralls |
There was a problem hiding this comment.
The PR looks good to me based on my limited experience of BackendV2. I would like to know whether I should update my PR #7643 to reflect the changes here after this PR is merged. I was initially planning to rename the backends in fake_backend_v2.py to avoid confusion with the new FakeBackendV2 I created, but perhaps that can be made in a follow up PR separately. What do you think? @mtreinish
| # import QubitProperties here to provide convenience alias for building a | ||
| # full target | ||
| from qiskit.providers.backend import QubitProperties # pylint: disable=unused-import | ||
|
|
There was a problem hiding this comment.
I don't fully understand the motivation of this line. Can you explain a bit more?
There was a problem hiding this comment.
It's a convenience thing since QubitProperties is now optionally part of a target I thought it would be good to have a single import for everything needed to make a taget. This is just to enable someone creating a Target object to do:
from qiskit.transpiler.target import Target, InstructionProperties, QubitPropertiesinstead of:
from qiskit.transpiler.target import Target, InstructionProperties
from qiskit.providers.backend import QubitProperties(although the latter will still work even with this.)
But, it is a bit weird to do this, so I'm not really tied to it.
There was a problem hiding this comment.
That makes a lot of sense. It's much more convenient this way.
| class FakeBackendSimple(BackendV2): | ||
| """A fake simple backend that wraps BasicAer to implement run().""" |
There was a problem hiding this comment.
This was not introduced by this PR but I think it would be good to explain in the docstring the difference between FakeBackendV2 and FakeBackendSimple.
Yeah if we merge this before #7643 it would be good to update the #7643 to include the
I think we can do that in a follow up to #7643. For #7643 I think it would be good if we just keep the PR to adding a v2 implementation of the fake backends (which will be a big enough change on its own). Cleanups like renaming things are small enough that they should be quick PRs. |
This commit removes an unecessary getter method to access the qubit_properties attribute of the target. The attribute was already public and we could get the qubit properties directly from the list stored in the target.
Oops, not sure how I forgot that. Added in d4e0fd4 |
Summary
This commit adds a new attribute to the target class for defining qubit
properties. This was originally not included in the target because
nothing in the transpiler supports using any qubit properties. However,
in a future PR I'm planning to modify the default heuristic layout score
used by VF2Layout to factor in T1 and/or T2 in addition to the
measurement error rate it's using now. To do this though we need to
ensure that we have these values available via the target. This commit
also is the first time we've had to expand the set of information that
can optionally be pecified in a target. I opted to just make a straight
addition here rather than versioning it. There should be no backwards
compatibility issue for users or provider maintainers on upgrade because
not specifying a qubit properties payload will not change any behavior
the Target and BackendV2 objects will behave exactly as before.
I was thinking we'd only use a version if we needed to make a backwards
incompatible change in the future. That way we can support both versions
of the class a the same time while we migrated to a newer interface. But
for simple additions like this that don't effect the behavior or cause
any issues with compatibility we can simply add them to the class.
Details and comments