-
Notifications
You must be signed in to change notification settings - Fork 68
A new algorithm_globals to replace use of qiskit.utils instance #33
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.
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
Conversation
Pull Request Test Coverage Report for Build 5868479771
💛 - Coveralls |
At present it seems there is an issue building what I expect the release note to look like. Locally, before pushing the code, they seem to build out fine and be what I expect. Here in CI the documentation zip just seems to have the release note from this PR and nothing of the prior release. The tutorials ones the release note page is entirely blank. Investigating.... I think the tutorials build is different since it has the script to ignore untagged release notes - in the apps this is only done for stable tutorials build so I removed it. That ought to make the documentation and tutorial zips the same in terms of release notes. That did make the difference go way as expected. Now the question of why its not including the prior notes from the 0.1.0 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks so much for taking care of the docs build issues too. I had to fix a minor merge conflict from #35 so I'll wait for the CI to build to assign the automerge label.
Summary
Fixes #21
Adds a new algorithm_globals here in qiskit_algorithms as part of the migration out of Qiskit. At present the instance here works with the instance in Qiskit, delegates to it, so that user code which may set the seed there still works as expected on the algorithms here. The code here has the same logic as the one in qiskit and that will be used when the Qiskit instance is removed by which time users should be using/seeding this instance directly.
The algorithm_globals introduced here only supports
random
andrandom_seed
.The
massive
property of the qiskit.utils version was only used in opflow so is not needed in this algorithm_globals.The qiskit.utils algorithm_globals also has a
num_processes
but this seems only being used by Qiskit Nature - it allows how many processes to be used by Qiskit's parallel map, something I think is more directly configurable now which back in Aqua times, when the globals came about I think it always used all local CPU cores. Anyway since this is all related to the Qiskit parallel_map usage, having some setting here, that is not even used by the algorithms seems out of place.Details and comments
This first commit just completes the first two items above. While locally I have the test cases updated too I am leaving them for now seeding the qiskit.utils instance to show that the algorithms still use this (indirectly) as many tests will fail without the RNG being seeded for a reproducible outcome. This works locally I am just assuring it does when run through CI here.
Ok CI passes e.g https://github.com/qiskit-community/qiskit-algorithms/actions/runs/5732026311/job/15534167826?pr=33 but it emits deprecation warnings about seeding the qiskit.utils instance e.g.
So updating to add altered tests that seed the instance here. CI now passes just the same but no longer are deprecation warnings like above being emitted.
Release note add. I updated the make files too as
make clean html
was not working and I expected it to - only manually deleting build and stubs would give me a clean run ahead of this change.