-
Notifications
You must be signed in to change notification settings - Fork 368
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
Add support for BackendV2 in NoiseModel.from_backend #1484
Conversation
72b9651
to
6294344
Compare
395c5eb
to
a198212
Compare
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.
Overall LGTM, some small inline comments. The only other thing is that we need to bump the minimum qiskit-terra version to 0.20.0 since some of the QubitProperties
usage was only added in 0.20.0
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def basic_device_readout_errors(properties): | ||
def basic_device_readout_errors(properties, target=None): |
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.
Should we make properties
optional here too? Right now you'll have to call this like basic_device_readout_errors(None, target=target)
to use this.
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.
Good point. Done in 9a6ac73
@@ -84,18 +102,43 @@ def basic_device_gate_errors(properties, | |||
standard_gates (bool): DEPRECATED, If true return errors as standard | |||
qobj gates. If false return as unitary | |||
qobj instructions (Default: None). | |||
warnings (bool): Display warnings (Default: True). | |||
warnings (bool): PLAN TO BE DEPRECATED, Display warnings (Default: True). |
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.
We should raise a PendingDeprecationWarning
if this is set. (Maybe change the default to None
and condition the warning on it not being None
.
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.
Done in 30c3930
target = copy.deepcopy(target) | ||
for op_name, qubits, value in gate_lengths: | ||
prop = target[op_name][qubits] | ||
prop.duration = value * _NANOSECOND_UNITS[gate_length_units] |
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.
Does _NANOSECOND_UNITS
convert to ns or to seconds? The units for InstructionProperties.duration
should be in seconds.
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.
Good catch! Fixed at bad25ec
prop = target[op_name][qubits] | ||
prop.duration = value * _NANOSECOND_UNITS[gate_length_units] | ||
target.update_instruction_properties(op_name, qubits, prop) | ||
all_qubit_properties = backend.target.qubit_properties |
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.
You can use the backend.qubit_properties()
method as a backup here if needed since the qubit_properties
target attribute only was added in 0.20.0, so it's possible that a BackendV2
instance from 0.19.x is missing the target implementation but has it on the backend. This pretty unlikely though so it's not required.
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've just bumped the minimum terra version to 0.20.0 in a46948b.
test/terra/noise/test_noise_model.py
Outdated
def test_noise_model_from_backend_v2(self): | ||
circ = QuantumCircuit(2) | ||
circ.x(0) | ||
circ.x(1) | ||
circ.measure_all() | ||
|
||
backend = mock.FakeBackendV2() | ||
noise_model = NoiseModel.from_backend(backend) | ||
circ = transpile(circ, backend, optimization_level=0) | ||
result = AerSimulator().run(circ, noise_model=noise_model).result() | ||
self.assertTrue(result.success) |
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.
Do you want to add some tests that use fake devices from terra like FakeLagosV2 or 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.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish Thank you for your review. I've updated the code accordingly. Could you take a second look? |
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 for making the updates so quickly. Just an inline question and a suggested improvement to the warning message. Otherwise I think this is good to go.
else: | ||
# create from Target | ||
for q in range(target.num_qubits): | ||
prop = target['measure'][(q,)] |
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'm not sure we want to do this with a get item here without checking that measure
is defined in the target. I think in practical terms almost every target will have this defined but it's not guaranteed.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish I've got an error during |
It's caused by the recent sphinx release, #1548 should have fixed it |
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.
LGTM, sorry for the slow review. Thanks for updating things
Summary
Fixes #1477 by adding support for
BackendV2
inNoiseModel.from_backend
.Details and comments
Updates
basic_device_readout_errors
andbasic_device_gate_errors
functions innoise.device.models
module so that they can take a Target instead of a BackendProperties.Not supporting
standard_gates
andwarnings
options for BackendV2 because those options are (planned to be) deprecated.Requiring terra#7736 as a prerequisite.