Qiskit-Extensions / qiskit-experiments

Qiskit Experiments
https://qiskit-extensions.github.io/qiskit-experiments/
Apache License 2.0
151 stars 122 forks source link

Update SingleTransmonTestBackend for Qiskit Dynamics 0.5.0 #1427

Closed wshanks closed 3 months ago

wshanks commented 3 months ago

Dynamics 0.5.0 changed the arguments it expects for Solver and underlying models. In particular, instead of evaluation_mode, array_library and vectorized are expected. See https://qiskit-extensions.github.io/qiskit-dynamics/release_notes.html#release-notes-0-5-0

wshanks commented 3 months ago

@DanPuzzuoli I noticed some reduction in execution time for the tests using Dynamics. That's probably performance improvements in the new release and not me choosing the wrong translation of the solver options, right? The difference for individual tests is like 3.7 sec vs 4.0 sec (test.library.calibration.test_rough_amplitude.TestRoughAmpCal.test_update) and 1.9 sec vs 3.2 sec (test.library.calibration.test_rough_frequency.TestRoughFrequency.test_update_calibrations). I didn't get a lot of statistics for those numbers.

DanPuzzuoli commented 3 months ago

@DanPuzzuoli I noticed some reduction in execution time for the tests using Dynamics. That's probably performance improvements in the new release and not me choosing the wrong translation of the solver options, right? The difference for individual tests is like 3.7 sec vs 4.0 sec (test.library.calibration.test_rough_amplitude.TestRoughAmpCal.test_update) and 1.9 sec vs 3.2 sec (test.library.calibration.test_rough_frequency.TestRoughFrequency.test_update_calibrations). I didn't get a lot of statistics for those numbers.

Hmm interesting - to be honest we didn't do any actual benchmarking of the whole package with NumPy execution. I know that @chriseclectic had done some benchmarking of Arraylias when he wrote it and found that it was faster than previous the Array wrapping in Dynamics (I forget by how much), so it's believable to me that there are improvements. (Aside: Perhaps we should have looked into this more and potentially advertised it in the release notes :D, but I didn't think about it much; for JAX execution all of these overheads get eliminated by compilation anyway. That being said, this could, and probably does, improve compilation time.)

Just looking at the changes I think they are correct, so I don't think there's any issue there. Setting array_library should not impact simulation results, it should only impact the speed, or potentially cause it to outright fail if there was some fundamental underlying incompatibility, e.g. between the specified array_library in the model and the input state type. Similarly, setting vectorized to True or False should either have no impact on the simulation results, or it will cause an outright failure (e.g. simulating a SuperOp requires setting vectorized=True, but a DensityMatrix or Statevector input can be simulated with either vectorized=True or vectorized=False).

wshanks commented 3 months ago

That is a good point. There are also framework/package_deps.py and warnings.py (which just has warnings about optional dependencies). Maybe it could all move into package_deps.py. I will try that in a follow up.