Fix bug in result creation and add instantiation of EFGSS to how-to #268
Conversation
caleb-johnson
added
bug
Pull Request Test Coverage Report for Build 5326993381
💛 - Coveralls |
| @@ -9,15 +9,6 @@ | |||
| "In this guide, we apply Entanglement Forging to compute the energy of a $\\mathrm{H}_2\\mathrm{O}$ molecule. We reduce the number of orbitals in the problem, in turn reducing the number of qubits needed in each circuit, following the logic given in the [explanatory material](../explanation/index.rst)." | |||
| ] | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Trimming down unnecessary explaining in how-to's here.
| @@ -43,6 +44,7 @@ | |||
| from .entanglement_forging_operator import EntanglementForgingOperator | |||
| from .entanglement_forging_ground_state_solver import ( | |||
| EntanglementForgingGroundStateSolver, | |||
| EntanglementForgingResult, | |||
There was a problem hiding this comment.
Shouldnt have to go all the way into the EFGSSolver to get the main result type of this tool. Adding it here
| @@ -317,14 +316,11 @@ def solve( | |||
|
|
|||
| # Create the EntanglementForgingResult from the results from the | |||
| # results of eigenvalue minimization and other meta information | |||
| min_eigsolver_result = MinimumEigensolverResult() | |||
There was a problem hiding this comment.
unnecessary and also buggy. Just removing this unnecessary conversion fixes the bug
| "driver = PySCFDriver(\n", | ||
| " f\"O 0.0 0.0 0.0; H {H1_x} 0.0 0.0; H {H2_x} {H2_y} 0.0\", basis=\"sto6g\"\n", | ||
| ")\n", | ||
| "driver.run()\n", | ||
| "problem = driver.to_problem(basis=ElectronicBasis.AO)" |
There was a problem hiding this comment.
This actually needs to be instantiated in the MO basis, which is the default
| "source": [ | ||
| "solver = EntanglementForgingGroundStateSolver(\n", | ||
| " ansatz=ansatz,\n", | ||
| " orbitals_to_reduce=orbitals_to_reduce,\n", |
There was a problem hiding this comment.
Need to tell the EFGSS how to split the observable to match the reduced ansatz via the orbitals_to_reduce arg. This is a crucial step and should be included here in this how-to
| result.eigenvalues = np.asarray([optimal_evaluation.eigenvalue]) | ||
| result.eigenstates = [(optimal_evaluation.eigenstate, None)] |
There was a problem hiding this comment.
Why are eigenvalues and eigenstates plural? There is an eigenvalue (singular) property on MinimumEigensolverResult but I don't see eigenstates there or in EntanglementForgingResult. And why is eigenstates a 2-tuple? (Interesting to note that this didn't break any tests.)
There was a problem hiding this comment.
Why are
eigenvaluesandeigenstatesplural? There is aneigenvalue(singular) property onMinimumEigensolverResultbut I don't seeeigenstatesthere or inEntanglementForgingResult. And why iseigenstatesa 2-tuple? (Interesting to note that this didn't break any tests.)
The groundstate and groundenergy properties (the ones we really care about) are derived from these fields. groundenergy is the first element in the eigenvalues list, and groundstate is the first index of the first index of eigenstates. It didn't break any tests bc we've been using EigenstateResult as a base class the whole time.
Your comment brings up a larger question of whether we can break the dependency on this class. This would be in-line with all of the dependencies we've broken with Nature in the EFGSSolver, so I think the answer is yes. I've opened #275 to track this.
There was a problem hiding this comment.
Thanks. You clarified the source of my confusion, namely, that EntanglementForgingResult derives from EigenstateResult (which is in Qiskit Nature), not MinimumEigensolverResult (which is in Qiskit Terra).
Co-authored-by: Jim Garrison <[email protected]>
Resolves #132
Resolves #247
This PR fixes a bug which was preventing the EFResults from being properly populated in certain cases, such as when we include
orbitals_to_reduceargument toEFGSSolverIt also introduces an instantiation of the
EFGroundStateSolverin the bitstring reduction how-to. Users need to pass in the orbitals to reduce here, as well as the ansatz, so that the observable can be properly reduced as well. This has been a point of confusion, so adding this step should clear this confusion up.