Update BaseEstimator import to support qiskit 2.0 #2327
Conversation
|
In my testing with Aer and Qiskit 2.0, this line is also problematic. Do you want to address it here as well? The problem addressed by the fix in this PR and the line I pointed out are both problems with trying to do |
|
Ah, there is a helpful test run that now picks up the Qiskit rc and encounters further issues: |
45d1bbe to
e2d93d5
Compare
…te-primitive-imports
… but remove use in tests
…epted as a gate definition
…s to c_if and added a conditional skip depending on qiskit version
a085662 to
bb1869a
Compare
|
A couple of unit tests with fractional gates were failing because of Qiskit/qiskit#14002. Given that this PR is blocking other efforts, I added a co 10000 nditional skip for these tests until the issue is addresed. |
|
The estimator unit tests are currently failing because of another bug in qiskit: Qiskit/qiskit#14003. A fix is on the way. In the meantime, I have chosen to skip transpilation in that specific test, as it's the part of the code that triggers the qiskit bug. |
…ng up the circuit to an arbitrary coupling map, which is something independent of the enable_truncation flag. The test now skips transpilation and exclusively tests the truncation (with enable_truncation=True, the test returns 2 instead of 4 qubits)
| @@ -322,7 +322,7 @@ def test_result_order(self): | |||
| qc2.ry(np.pi / 2 * param, 0) | |||
| qc2.measure_all() | |||
|
|
|||
| estimator = Estimator(approximation=True) | |||
| estimator = Estimator(approximation=True, skip_transpilation=True) | |||
There was a problem hiding this comment.
Try run_options = {'shots': None} to get exact values for the circuits that result in 0.
Alternative: change qc2 to give -1 instead of 0, then you won't have shot noise either. This can be done by changing np.pi / 2 to np.pi in the circuit, or [1] to [2] in the parameter.
(Estimator has no shot noise when the expectation value is exactly -1 or 1)
There was a problem hiding this comment.
Thanks! I haven't used the aer estimator in a while and forgot how to set these.
There was a problem hiding this comment.
This option is documented at
qiskit-aer/qiskit_aer/primitives/estimator.py
Lines 68 to 75 in 076fe00
skip_transpilation=True.
There was a problem hiding this comment.
It's likely. Something to look at in a follow-up.
| self.assertEqual(result.results[0].header.metadata, metadata) | ||
| try: | ||
| out_metadata = result.results[0].header.metadata | ||
| except AttributeError: |
There was a problem hiding this comment.
Perhaps using hasattr is preferable to failing, although it's not that important.
|
|
||
| result = backend.run(circuit, shots=100).result() | ||
| metadata = result.results[0].metadata | ||
| self.assertEqual(metadata["num_qubits"], 10) | ||
| self.assertEqual(metadata["num_qubits"], 4) |
There was a problem hiding this comment.
This seems to subtly change the test. Previously, there was a difference between the num_qubits value (which seems to have been determined by the now-extinct coupling map) and the number of active qubits - I believe this difference results from how truncation works in the CPP level. However, I did not manage to recreate this desired behavior yet when working with qiskit 2.0 so we might want to keep this fix as it is for now.
There was a problem hiding this comment.
I actually think there is no difference in the way truncation works, the only actual difference I have seen is in the way the transpiler works between 1.4 and 2.0, but that is intentional because we added extra consistency checks between the inputs. The original test provided a backend with 5 qubits and a coupling map with 10, and this is something that transpile now processes differently. I tested transpiling only with the coupling map and the output metadata matches the original test. So maybe we could do that.
| @@ -595,6 +595,10 @@ def test_shot_branching_kraus_circuit_noise(self, method, device): | |||
| # --------------------------------------------------------------------- | |||
| # Test conditional | |||
| # --------------------------------------------------------------------- | |||
| @unittest.skipUnless( | |||
There was a problem hiding this comment.
Can these tests be fixed to work with the new if mechanism in qiskit 2.0?
There was a problem hiding this comment.
To some extent, yes. From what I have seen, the new if mechanism is only supported by some simulator methods (for example, statevector doesn't support it). I think that moving forward the tests would have to be refactored and only test conditionals with the supported simulator methods, but this is a bit tricky at the moment because we still want to support the "old" path, and there are already some tests for the new if, so I figured that the best intermediate solution would be the conditional skip. I can open an issue to keep track of this.
There was a problem hiding this comment.
An issue seems like the correct way to proceed, thank you.
Summary
Qiskit/qiskit#13877 removes the non-versioned primitive V1 base class aliases. It looks like the
AerSamplerwas already using the versioned base class, butAerEstimatorwasn't. This PR updates theBaseEstimatorimport toBaseEstimatorV1.Details and comments
The PR has been extended to handle more incompatibilities with 2.0.