Need to fix the inconsistent part between the base classes and the reference implementations of Primitives

[t-imamichi]

What is the expected enhancement?

There are some different part between the base classes and the reference implementations of Primitives. I think we need to make them consistent. Since removing existing features is more difficult than adding new features, I think it would be good to keep the reference implementation as simple as possible as the first version.

1. The reference implementation can broadcast parameters only if there are 1 circuit and 1 observable for estimator, 1 circuit for sampler. This is not documented in the base classes. I feel this rule is a bit complicated and this rule is not mentioned in the example of the base classes.

   • corresponding codes: estimator, sampler

# first example
params = [[0, 0], [1, 1], [2, 2]]
with Estimator(qc, op) as estimator:
  result = estimator(parameter_values=params) # OK with the current impl: result.values = expval(qc, op, [0, 0]), expval(qc, op, [1, 1]), expval(qc, op, [2, 2])
  result = estimator([0] * params.shape[0], [0] * params.shape[0], parameter_values=params)  # equivalent code w/o broadcasting 

with Estimator([qc] * 2, [op] * 2) as estimator:
  result = estimator(parameter_values=params) # Error

2. circuit_indices and observable_indices are required in the base classes, but the reference implementation allows optional, i.e., they use all circuits or all observables if indices are None.

   • corresponding codes: estimator, sampler

https://github.com/Qiskit/qiskit-terra/blob/688cf6abe4ec0a2f843a63135cc6c3e9a497b2c3/qiskit/primitives/base_estimator.py#L199-L203 https://github.com/Qiskit/qiskit-terra/blob/688cf6abe4ec0a2f843a63135cc6c3e9a497b2c3/qiskit/primitives/estimator.py#L60-L64

# second example
params = [[0, 1], [2, 3]]
with Estimator([qc1, qc2], [op1, op2]) as estimator:
  result = estimator(parameter_values=params)  # OK with the current impl: result.values = expval(qc1, op1, [0, 1]), expval(qc2, op2, [2, 3])
  # the above code is similar to the broadcasting rule (see the first example), but behaves in a different way
  result = estimator([0, 1], [0, 1], params)  # equivalent code

The combination of these two rules is tricky. According to the first example, estimator(parameter_values=params) in the second example looks broadcasting params, but it actually takes zip of quantum circuits and observables.

[ikkoham]

Basically, extending on the subclass side is fine as long as it does not violate Liskov's substitution principle. However, it would be more convenient if it were available from other samplers and estimators. But, extending the base class specification raises the hurdle for subclass implementation. This is keeping in mind that third parties will implement them.

I therefore suggest the following approach:

1. Remove allow broadcasting and allow optional from the reference implementation.

2. Implement class decorator like:

   @allow_optional
   @allow_broadcasting
   def CustomSampler(Sampler):
       pass
   

[t-imamichi]

If we keep the optional parameters of the reference implementation, I suggest to write docstrings about the the extended part of the reference implementation such as the behavior of optional circuit_indices and observable_indices.

[levbishop]

My preference is to remove the broadcasting and optional behaviours. I find having to write an explicit [0]*n or [[]] not very painful and makes the nature of the broadcast very explicit and easy to modify eg if you have a second circuit to broadcast over just change to [1]*n

[t-imamichi]

Thank you, @levbishop. I have already removed the broadcasting #7837. If there is no objection, we will remove optional behavior as well.

[t-imamichi]

8065 removed the optional behavior.

[Cryoris]

I see the advantage of being explicit with the broadcasting: you're always aware of what's being calculated and maybe this points out an error if you have lists of differing lengths. However in the algorithms we very often have the setting where we have the same circuit (and observable) and them for different parameter values. Right now this forces us to write something like

def evaluate(parameters: np.ndarray | list[np.ndarray]):
    if not isinstance(parameters, list):
        parameters = [parameters]
    batchsize = np.asarray(parameters).shape[0]
    result = estimator.run(batchsize * [circuit], batchsize * [operator], parameters).result()

which is a bit cumbersome (and the same code get's repeated a lot). We need to handle a single array of parameters and an arbitrary number of parameter-arrays here, since different optimizers can submit batches of parameters at once.

It would be much nicer if instead we could write something like

def evaluate(parameters: np.ndarray | list[np.ndarray]):
    result = estimator.run(circuit, operator, parameters).result()

Maybe we can also support broadcasting only if the circuit and operator are not a list, that might still be clear enough as behaviour? 🙂

[garrison]

For anyone interested in continuing the conversation about parameter broadcasting, there is an RFC for changes to the Estimator, with discussion at https://github.com/Qiskit/RFCs/pull/51.

[t-imamichi]

I close this because the discussion is obsolete