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.

Sign up for GitHub

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

Fix Backend primitive classes for BackendV1 with no max_experiments #9069

Merged
merged 3 commits into from
Nov 3, 2022
Merged

Fix Backend primitive classes for BackendV1 with no max_experiments #9069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
928c2de
Fix Backend primitive classes for BackendV1 with no max_experiments
mtreinish Nov 3, 2022
050d1ba
Update test/python/primitives/test_backend_estimator.py
mtreinish Nov 3, 2022
745d0af
Merge branch 'main' into fix-backendv1-access
mergify[bot] Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion qiskit/primitives/backend_estimator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def _run_circuits(
metadata.append(circ.metadata)
circ.metadata = {}
if isinstance(backend, BackendV1):
max_circuits = backend.configuration().max_experiments
max_circuits = getattr(backend.configuration(), "max_experiments", None)
Comment on lines 58 to +59
Copy link
Member

Choose a reason for hiding this comment

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

Not ideal that we're effectively switching on out-of-interface behaviour, but probably we're in a situation for BackendV1 where the ship has sailed (and we can always backdate and say it's an "optional" field).

Copy link
Member Author

@mtreinish mtreinish Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I agree it's weird but I think it's probably defined behavior for the interface at this point (it's just really bad/weird behavior). I did check the schema that BackendConfiguration was originally based on and max_experiments isn't a required field: https://github.com/Qiskit/ibm-quantum-schemas/blob/main/schemas/backend_configuration_schema.json#L32 which was mirrored in the class when the schema was still coupled with the class definition. It's definitely a weird interface though and why I've been so keen to get everything on BackendV2.

Copy link
Member

@pedrorrivero pedrorrivero Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have taken a look at BackendConfiguration and there seems to be a lot of code like:

...
if max_experiments:
    self.max_experiments = max_experiments
if sample_name is not None:
    self.sample_name = sample_name
...

Wouldn't it make things easier to use properties and return None by default? Seems more stable than needing hasattr() checks, and it is not a breaking change...

elif isinstance(backend, BackendV2):
max_circuits = backend.max_circuits
if max_circuits:
Expand Down
7 changes: 7 additions & 0 deletions releasenotes/notes/fix-backend-primitive-no-max-experiments-e2ca41ec61de353e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
fixes:
- |
Fixed an issue with the backend primitive classes :class:`~.BackendSampler`
and :class:`~.BackendEstimator` which prevented running with a
:class:`~.BackendV1` instance that does not have a ``max_experiments``
field set in its :class:`~.BackendConfiguration`.
25 changes: 25 additions & 0 deletions test/python/primitives/test_backend_estimator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,31 @@ def test_job_size_limit_v1(self):
estimator.run([qc] * k, [op] * k, params_list).result()
self.assertEqual(run_mock.call_count, 10)

def test_no_max_circuits(self):
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
"""Test BackendEstimator works with BackendV1 and no max_experiments set."""
backend = FakeNairobi()
config = backend.configuration()
del config.max_experiments
backend._configuration = config
backend.set_options(seed_simulator=123)
qc = RealAmplitudes(num_qubits=2, reps=2)
op = SparsePauliOp.from_list([("IZ", 1), ("XI", 2), ("ZY", -1)])
k = 5
params_array = np.random.rand(k, qc.num_parameters)
params_list = params_array.tolist()
params_list_array = list(params_array)
estimator = BackendEstimator(backend=backend)
target = estimator.run([qc] * k, [op] * k, params_list).result()
with self.subTest("ndarrary"):
result = estimator.run([qc] * k, [op] * k, params_array).result()
self.assertEqual(len(result.metadata), k)
np.testing.assert_allclose(result.values, target.values, rtol=0.2, atol=0.2)

with self.subTest("list of ndarray"):
result = estimator.run([qc] * k, [op] * k, params_list_array).result()
self.assertEqual(len(result.metadata), k)
np.testing.assert_allclose(result.values, target.values, rtol=0.2, atol=0.2)


if __name__ == "__main__":
unittest.main()