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

Fix bug in result creation and add instantiation of EFGSS to how-to #268

Merged
merged 13 commits into from
Jun 20, 2023
3 changes: 3 additions & 0 deletions circuit_knitting/forging/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
EntanglementForgingOperator
EntanglementForgingAnsatz
EntanglementForgingGroundStateSolver
EntanglementForgingResult

Decomposition Functions
=======================
Expand All @@ -43,6 +44,7 @@
from .entanglement_forging_operator import EntanglementForgingOperator
from .entanglement_forging_ground_state_solver import (
EntanglementForgingGroundStateSolver,
EntanglementForgingResult,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldnt have to go all the way into the EFGSSolver to get the main result type of this tool. Adding it here

)
from .cholesky_decomposition import cholesky_decomposition, convert_cholesky_operator

Expand All @@ -51,6 +53,7 @@
"EntanglementForgingKnitter",
"EntanglementForgingOperator",
"EntanglementForgingGroundStateSolver",
"EntanglementForgingResult",
"cholesky_decomposition",
"convert_cholesky_operator",
]
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import scipy
import numpy as np

from qiskit.algorithms.minimum_eigensolvers import MinimumEigensolverResult
from qiskit.algorithms.optimizers import SPSA, Optimizer, OptimizerResult
from qiskit_nature.second_q.problems import (
ElectronicStructureProblem,
Expand Down Expand Up @@ -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()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unnecessary and also buggy. Just removing this unnecessary conversion fixes the bug

min_eigsolver_result.eigenvalue = optimal_evaluation.eigenvalue
min_eigsolver_result.eigenstate = optimal_evaluation.eigenstate
result = EntanglementForgingResult.from_minimum_eigensolver_result(
min_eigsolver_result
)
result.energy_shift = self._energy_shift
result = EntanglementForgingResult()
result.eigenvalues = np.asarray([optimal_evaluation.eigenvalue])
result.eigenstates = [(optimal_evaluation.eigenstate, None)]
Comment on lines +320 to +321
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

https://github.com/qiskit-community/qiskit-nature/blob/14ff6490c09af7091b90d1c38a99815a92b93a83/qiskit_nature/second_q/problems/eigenstate_result.py#L62-L63

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

result.history = self._history.evaluations
result.energy_shift = self._energy_shift
result.elapsed_time = elapsed_time

# Close any runtime sessions
Expand Down
57 changes: 36 additions & 21 deletions docs/entanglement_forging/how-tos/freeze-orbitals.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,9 @@
"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)."
]
},
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Trimming down unnecessary explaining in how-to's here.

"cell_type": "markdown",
"metadata": {},
"source": [
"### Import the relevant modules\n",
"\n",
"First, we import the relevant modules. The imports are similar to the introductory tutorial, but this time, we also import the `reduce_bitstrings` function from `circuit_knitting.utils`."
]
},
{
"cell_type": "code",
"execution_count": 1,
"execution_count": null,
"metadata": {
"ExecuteTime": {
"end_time": "2021-04-27T13:41:07.878080Z",
Expand All @@ -33,11 +24,12 @@
"import numpy as np\n",
"\n",
"from qiskit.circuit import QuantumCircuit, Parameter\n",
"from qiskit.algorithms.optimizers import COBYLA\n",
"from qiskit_nature.second_q.drivers import PySCFDriver\n",
"from qiskit_nature.second_q.problems import ElectronicBasis\n",
"\n",
"from circuit_knitting.forging import (\n",
" EntanglementForgingAnsatz,\n",
" EntanglementForgingGroundStateSolver,\n",
")\n",
"from circuit_knitting.utils import reduce_bitstrings"
]
Expand Down Expand Up @@ -70,11 +62,12 @@
"H2_x = radius_2 * np.cos(np.pi / 180 * thetas_in_deg)\n",
"H2_y = radius_2 * np.sin(np.pi / 180 * thetas_in_deg)\n",
"\n",
"# Create an ElectronicStructureProblem from the molecule using PySCFDriver\n",
"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)"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This actually needs to be instantiated in the MO basis, which is the default

"problem = driver.to_problem()"
]
},
{
Expand All @@ -84,13 +77,6 @@
"### Prepare the bitstrings and the ansatz"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"The ansatz for Entanglement Forging consists of a set of input bitstrings and a parameterized circuit. (See the [explanatory material](../explanation/index.rst) section of the documentation for additional background on the method.) For this demo, we will use the same bitstrings and ansatz for both the U and V subsystems. "
]
},
{
"cell_type": "code",
"execution_count": 3,
Expand Down Expand Up @@ -133,6 +119,13 @@
"hop_gate.draw()"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"Create the ansatz with a reduced set of bitstrings."
]
},
{
"cell_type": "code",
"execution_count": 4,
Expand Down Expand Up @@ -201,18 +194,40 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"From here, the problem can be solved following the same steps as in the [tutorials](../tutorials/index.rst)."
"Create the ``EntanglementForgingGroundStateSolver``, and specify which orbitals to reduce in the observable."
]
},
{
"cell_type": "code",
"execution_count": 5,
"metadata": {},
"outputs": [],
"source": [
"optimizer = COBYLA(maxiter=100)\n",
"\n",
"solver = EntanglementForgingGroundStateSolver(\n",
" ansatz=ansatz,\n",
" optimizer=optimizer,\n",
" orbitals_to_reduce=orbitals_to_reduce,\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

")"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"From here, the problem can be solved following the same steps as in the [tutorials](../tutorials/index.rst)."
]
},
{
"cell_type": "code",
"execution_count": 6,
"metadata": {},
"outputs": [
{
"data": {
"text/html": [
"<h3>Version Information</h3><table><tr><th>Qiskit Software</th><th>Version</th></tr><tr><td><code>qiskit-terra</code></td><td>0.23.3</td></tr><tr><td><code>qiskit-aer</code></td><td>0.12.0</td></tr><tr><td><code>qiskit-ibmq-provider</code></td><td>0.20.2</td></tr><tr><td><code>qiskit-nature</code></td><td>0.5.2</td></tr><tr><th>System information</th></tr><tr><td>Python version</td><td>3.8.16</td></tr><tr><td>Python compiler</td><td>Clang 14.0.6 </td></tr><tr><td>Python build</td><td>default, Mar 1 2023 21:19:10</td></tr><tr><td>OS</td><td>Darwin</td></tr><tr><td>CPUs</td><td>8</td></tr><tr><td>Memory (Gb)</td><td>32.0</td></tr><tr><td colspan='2'>Tue Apr 11 19:14:09 2023 CDT</td></tr></table>"
"<h3>Version Information</h3><table><tr><th>Qiskit Software</th><th>Version</th></tr><tr><td><code>qiskit-terra</code></td><td>0.24.0</td></tr><tr><td><code>qiskit-aer</code></td><td>0.12.0</td></tr><tr><td><code>qiskit-ibmq-provider</code></td><td>0.20.2</td></tr><tr><td><code>qiskit-nature</code></td><td>0.6.0</td></tr><tr><th>System information</th></tr><tr><td>Python version</td><td>3.8.16</td></tr><tr><td>Python compiler</td><td>Clang 14.0.6 </td></tr><tr><td>Python build</td><td>default, Mar 1 2023 21:19:10</td></tr><tr><td>OS</td><td>Darwin</td></tr><tr><td>CPUs</td><td>8</td></tr><tr><td>Memory (Gb)</td><td>32.0</td></tr><tr><td colspan='2'>Sat Jun 17 11:51:45 2023 CDT</td></tr></table>"
],
"text/plain": [
"<IPython.core.display.HTML object>"
Expand Down
6 changes: 6 additions & 0 deletions releasenotes/notes/ef-results-bug-5aa84ca5611dd827.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
fixes:
- |
Fixed a bug in :class:`~circuit_knitting.forging.EntanglementForgingGroundStateSolver` which was causing
AttributeErrors when instantiating the `~circuit_knitting.forging.EntanglementForgingResult` in certain
caleb-johnson marked this conversation as resolved.
Show resolved Hide resolved
conditions, such as when reducing the orbitals over which to solve.
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ def test_entanglement_forging_vqe_hydrogen(self):
# Solve for the ground state energy
results = solver.solve(problem)
ground_state_energy = results.groundenergy + results.energy_shift

print(results.groundenergy)
print(results.energy_shift)
# Ensure ground state energy output is within tolerance
self.assertAlmostEqual(ground_state_energy, -1.121936544469326)