Skip to content
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

Migrating qiskit_algorithms #817

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

edoaltamura
Copy link
Collaborator

Summary

Qiskit algorithms, which Qiskit machine-learning (ML) depends on, is no longer officially supported as of qiskit-community/qiskit-algorithms@4247d06. To continue improving the subset of Algorithms features that are relevant to Qiskit machine-learning, it has been proposed to migrate those from Algorithms to ML.

This PR implements the merge of the subset of Qiskit Algorithms features needed for ML, as described in #811.

Closes #811.

  • qiskit_algorithms/gradients -> qiskit_machine_learning/gradients
  • qiskit_algorithms/optimizers -> qiskit_machine_learning/optimizers
  • qiskit_algorithms/state_fidelities -> qiskit_machine_learning/state_fidelities
  • qiskit_algorithms/utils partially merged with qiskit_machine_learning/utils

Details and comments

The unit tests are also merged following the same structure as in the source directory.

  • qiskit_algorithms/tests/gradients -> qiskit_machine_learning/tests/gradients
  • qiskit_algorithms/tests/optimizers -> qiskit_machine_learning/tests/optimizers
  • qiskit_algorithms/tests/state_fidelities -> qiskit_machine_learning/tests/state_fidelities
  • qiskit_algorithms/tests/utils partially merged with qiskit_machine_learning/tests/utils

Once the merge is complete, the codebase will be ready for a targeted attempt at resolving:

@edoaltamura edoaltamura added priority: high type: design 📐 Relevant to code architecture dependencies Pull requests that update a dependency file labels Jul 11, 2024
@woodsp-ibm
Copy link
Member

I skimmed things very quickly but one thing it looks like you will need is to modify the main init docstring to include any new modules for the main table of contents there.

@woodsp-ibm
Copy link
Member

For all the copyrights, you know you can do a make copyright locally and it will fix them all? I guess this needs to be done with the way they have been brought over right - I think its possible to bring them over with history, more effort/fuss of course, but I guess that's all there in algorithms should any need it for any reason.

@woodsp-ibm
Copy link
Member

Maybe a release note outlining the changes with perhaps a migration guide too around what to do that the release note can also link too.

As this stands is it considered a breaking change? I guess, for instance an optimizer instance from algorithms would still work since the function/logic is the same. But without any logic here to detect this no sort of user warning would be emitted to notify them they ought to be be switching to the version here. I.e. logically I think at present it still supports either the migrated logic here or the equivalent in algorithms even if the types are not explicitly coded for that in the changes here

@edoaltamura
Copy link
Collaborator Author

Thanks for the input, Steve. I fixed the copyright year and now going through the outstanding mypy errors. Then, I'll move over to the docstrings and release notes.

This should not be a breaking change because the objects generated from Algorithms itself would be equivalent to those now generated in ML. But it would be a good idea to produce warnings or messages at specific points in the logic to ensure users are aware of this change as we consolidate the library.

I can discuss the merge with Git history, however it might be more complex after changing the absolute/relative imports in some of the files.

@edoaltamura edoaltamura added this to the 0.8.0 milestone Jul 15, 2024
@declanmillar
Copy link
Contributor

Hey, @edoaltamura. Are you still working on this? It might be work marking it as a Draft until you think it's ready for review.

@woodsp-ibm
Copy link
Member

I will note, that presently with the main branch of Qiskit, that is shortly to be released as 1.2.0 that there is an issue with the gradients/utils.py method _make_lin_comb_gradient_circuit that is copied over here in this PR - there is a "fix" in Qiskit Algorithms qiskit-community/qiskit-algorithms#188 but I was holding off merging at present in case the underlying logic in Qiskit is remediated before release and this once again works. There is more info in that PR - also Julien did a PR to that PR where he changed the logic to use the DAG instead which works and whatever the Rust changes related issue that caused the existing logic to fail does not get triggered this way with the DAG.

@declanmillar
Copy link
Contributor

declanmillar commented Jul 24, 2024

Is it necessary for us to copy over all these files from algorithms when we're planning to cull most of them to reduce the maintenance load? I thought the intention described in #811 was to remove all but the SPSA gradient from that submodule. It seems counterproductive to fix issues in these files when they'll be removed soon.

@declanmillar
Copy link
Contributor

Regarding the spelling issues, to save time, we could do a union of the dictionary sets in qiskit-algorithms and qiskit-machine-learning. This will introduce some redundancies, but they should all be relevant words.

@woodsp-ibm
Copy link
Member

Is it necessary for us to copy over all these files from algorithms when we're planning to cull

I just saw that libcomb gradient had been copied here and noted the issue with present Qiskit main that fails the nightly CI unit tests in CI where it tests against Qiskit main branch. In terms of the gradients it does seem to be a subset copied here of what is in algorithms, but I do recall the statement about SPSA one being the only reasonable choice for hardware.

@woodsp-ibm
Copy link
Member

Regarding the spelling issues, to save time, we could do a union of the dictionary sets in qiskit-algorithms and qiskit-machine-learning. This will introduce some redundancies, but they should all be relevant words.

My recollection of doing something in the past was to remove the custom dict let it create a list of all misspelled words and then sort the resultant to create the custom dict. I think I did this to create a dict to start with where I looked through for evident mispellings and removed them - in this case I guess all mispellings are correct and we would want the entire list.

@edoaltamura edoaltamura marked this pull request as draft July 25, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file priority: high type: design 📐 Relevant to code architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate qiskit_algorithms following end-of-support
5 participants