Skip to content

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Mar 20, 2024

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 wshanks force-pushed the dynamics-0.5 branch 2 times, most recently from 2a700a6 to 89af8c5 Compare March 20, 2024 21:43
@wshanks
Copy link
Collaborator Author

wshanks commented Mar 20, 2024

@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.

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 wshanks added the backport stable potential The issue or PR might be minimal and/or import enough to backport to stable label Mar 21, 2024
@DanPuzzuoli
Copy link

@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).

Copy link

@DanPuzzuoli DanPuzzuoli left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. We also have versioning-related code in database_service/utils.py. We could collect them all in the same place but it's not a big deal either way.

@wshanks
Copy link
Collaborator Author

wshanks commented Mar 21, 2024

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.

@wshanks wshanks enabled auto-merge March 21, 2024 16:22
@wshanks wshanks added this pull request to the merge queue Mar 21, 2024
Merged via the queue into qiskit-community:main with commit 6817445 Mar 21, 2024
mergify bot pushed a commit that referenced this pull request Mar 21, 2024
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

(cherry picked from commit 6817445)
wshanks added a commit to wshanks/qiskit-experiments that referenced this pull request Mar 21, 2024
Queries and tests of installed package versions were spread across a few
different modules. Here they are combined into one module
(`package_deps.py`) as suggested in
qiskit-community#1427.
@wshanks wshanks deleted the dynamics-0.5 branch March 21, 2024 16:53
wshanks added a commit to wshanks/qiskit-experiments that referenced this pull request Mar 21, 2024
Queries and tests of installed package versions were spread across a few
different modules. Here they are combined into one module
(`package_deps.py`) as suggested in
qiskit-community#1427.
coruscating pushed a commit that referenced this pull request Mar 21, 2024
…1427) (#1428)

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<hr>This
is an automatic backport of pull request #1427 done by
[Mergify](https://mergify.com).

Co-authored-by: Will Shanks <willshanks@us.ibm.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 26, 2024
Queries and tests of installed package versions were spread across a few
different modules. Here they are combined into one module
(`package_deps.py`) as suggested in
#1427.

---------

Co-authored-by: Helena Zhang <Helena.Zhang@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants