Skip to content

refactor: switch to using primitives in qiskit_nature/second_q stack#856

Merged
mrossinek merged 6 commits into
qiskit-community:mainfrom
mrossinek:primitive-based-algorithms
Oct 6, 2022
Merged

refactor: switch to using primitives in qiskit_nature/second_q stack#856
mrossinek merged 6 commits into
qiskit-community:mainfrom
mrossinek:primitive-based-algorithms

Conversation

@mrossinek
Copy link
Copy Markdown
Member

@mrossinek mrossinek commented Sep 30, 2022

Summary

This refactors the entire qiskit_nature/second_q stack to use the new Qiskit primitives.

Some further changes are required:

  • unify the VQE-Factory signatures with the new VQE (positional estimator, ansatz and optimizer)
    • do we want a default optimizier?
    • do we even want a default estimator?
  • refactor the EigenstateResult.eigenstates once Fix VQE result, add ansatz Qiskit/qiskit#8816 is merged
  • check the documentation strings for the validity after these changes

Details and comments

Closes #849

@mrossinek
Copy link
Copy Markdown
Member Author

Note: Neko is expected to fail here, since I bumped the required Terra version to 0.22.*. Once the release candidate is out, this should be fixed 🤔

@mrossinek mrossinek force-pushed the primitive-based-algorithms branch 2 times, most recently from a4f50ae to 74c0a4d Compare October 3, 2022 08:24
Comment thread qiskit_nature/second_q/problems/eigenstate_result.py Outdated
@mrossinek mrossinek requested a review from ElePT October 3, 2022 08:52
Comment thread qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py Outdated
@mrossinek
Copy link
Copy Markdown
Member Author

@ElePT @manoelmarques could either of you please take a look at updating the 12th tutorial: https://github.com/Qiskit/qiskit-nature/blob/main/docs/tutorials/12_deuteron_binding_energy.ipynb ?

@mrossinek
Copy link
Copy Markdown
Member Author

I am also noticing some flaky tests which use the Estimator primitive without any shots being specified.. I thought that I am setting the algorithm_globals.random_seed everywhere and I am also providing a fixed initial_point to the VQE everywhere. So I am at a bit of a loss with regards to the tests randomly failing.

@ElePT do you have any ideas what could be causing those failures?

@mrossinek mrossinek force-pushed the primitive-based-algorithms branch from 74c0a4d to 72ab0c6 Compare October 3, 2022 09:32
@mrossinek mrossinek force-pushed the primitive-based-algorithms branch from 0c5652b to 3fe8008 Compare October 5, 2022 09:40
@mrossinek
Copy link
Copy Markdown
Member Author

I realized just now that this was not a draft PR until now, so I cannot mark it as "ready for review"... So instead I'm leaving this comment to do just that 😄

There are two aspects to make note of:

@mrossinek mrossinek added this to the 0.5.0 milestone Oct 5, 2022
@coveralls
Copy link
Copy Markdown

coveralls commented Oct 5, 2022

Pull Request Test Coverage Report for Build 3196983293

  • 138 of 142 (97.18%) changed or added relevant lines in 20 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 85.554%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/algorithms/ground_state_solvers/ground_state_eigensolver.py 9 10 90.0%
qiskit_nature/second_q/properties/occupied_modals.py 4 5 80.0%
qiskit_nature/second_q/problems/eigenstate_result.py 48 50 96.0%
Files with Coverage Reduction New Missed Lines %
qiskit_nature/second_q/algorithms/ground_state_solvers/ground_state_solver.py 1 92.0%
Totals Coverage Status
Change from base Build 3172412780: 0.06%
Covered Lines: 17086
Relevant Lines: 19971

💛 - Coveralls

Comment thread README.md
Comment thread qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py Outdated
Comment on lines +101 to +105
if hasattr(raw_result, "eigenstates"):
result.eigenstates = [
(_statevector_to_circuit(Statevector(state)), None)
for state in raw_result.eigenstates
]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As things stands eigenstates comes from the classical Numpy solver so I am wondering why the conversion to a circuit here rather than perhaps giving back a Statevector. Someone wanting a circuit from it can always convert it themselves - is this something we use/want as a circuit?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The idea was to have a simple and consistent type for EigenstateResult.eigenstates. Namely list[tuple[QuantumCircuit, Sequence[float] | None]] meaning that it is a list of circuits + their optimal parameters (or None if non-parameterized circuit).

If a user needs the eigenstate, they can still obtain it from the EigenstateResult.raw_result object. Handling this consistently here, avoids another isinstance check when using the .eigenstates in other locations 👍

Comment thread qiskit_nature/second_q/problems/electronic_structure_problem.py
Comment thread qiskit_nature/second_q/problems/lattice_model_problem.py
Comment thread qiskit_nature/second_q/problems/vibrational_structure_problem.py
@mrossinek mrossinek merged commit b7cbbf6 into qiskit-community:main Oct 6, 2022
@mrossinek mrossinek deleted the primitive-based-algorithms branch October 6, 2022 12:55
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
…iskit-community#856)

* refactor: switch to using primitives in qiskit_nature/second_q stack

* refactor: split excited energy tests into subtests

* refactor: align VQE*Factory interface with new VQE

* fix: do not rely on optimal_parameters.values() order

* fix: update tutorials

* Apply changes based on code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update qiskit_nature.second_q.algorithms to use primitive-based algorithms

3 participants