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
@@ -25,6 +25,7 @@
EntanglementForgingOperator
EntanglementForgingAnsatz
EntanglementForgingGroundStateSolver
EntanglementForgingResult
Decomposition Functions
=======================
@@ -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
from qiskit_nature import settings
@@ -61,6 +63,7 @@
"EntanglementForgingKnitter",
"EntanglementForgingOperator",
"EntanglementForgingGroundStateSolver",
"EntanglementForgingResult",
"cholesky_decomposition",
"convert_cholesky_operator",
]
Original file line number Diff line number Diff line change
@@ -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,
@@ -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
103 changes: 37 additions & 66 deletions docs/entanglement_forging/how-tos/freeze-orbitals.ipynb
Original file line number Diff line number Diff line change
@@ -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)."
]
},
{
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,
@@ -34,10 +25,10 @@
"\n",
"from qiskit.circuit import QuantumCircuit, Parameter\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"
]
@@ -46,9 +37,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"### Instantiate the `ElectronicStructureProblem`\n",
"\n",
"Next, we set up the $\\mathrm{H}_2\\mathrm{O}$ molecule, specify the driver and converter, and instantiate an `ElectronicStructureProblem`."
"First, we set up the $\\mathrm{H}_2\\mathrm{O}$ molecule, specify the driver and converter, and instantiate an `ElectronicStructureProblem`."
]
},
{
@@ -70,73 +59,25 @@
"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

]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"### Prepare the bitstrings and the ansatz"
"problem = driver.to_problem()"
]
},
{
"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. "
"Create the ansatz with a reduced set of bitstrings."
]
},
{
"cell_type": "code",
"execution_count": 3,
"metadata": {},
"outputs": [
{
"data": {
"text/html": [
"<pre style=\"word-wrap: normal;white-space: pre;background: #fff0;line-height: 1.1;font-family: &quot;Courier New&quot;,Courier,monospace\"> ┌───┐┌───┐ ┌────────────┐ ┌───┐\n",
"q_0: ┤ H ├┤ X ├──■──┤ Ry(-1.0*θ) ├──■──┤ H ├\n",
" └───┘└─┬─┘┌─┴─┐├────────────┤┌─┴─┐└───┘\n",
"q_1: ───────■──┤ X ├┤ Ry(-1.0*θ) ├┤ X ├─────\n",
" └───┘└────────────┘└───┘ </pre>"
],
"text/plain": [
" ┌───┐┌───┐ ┌────────────┐ ┌───┐\n",
"q_0: ┤ H ├┤ X ├──■──┤ Ry(-1.0*θ) ├──■──┤ H ├\n",
" └───┘└─┬─┘┌─┴─┐├────────────┤┌─┴─┐└───┘\n",
"q_1: ───────■──┤ X ├┤ Ry(-1.0*θ) ├┤ X ├─────\n",
" └───┘└────────────┘└───┘ "
]
},
"execution_count": 3,
"metadata": {},
"output_type": "execute_result"
}
],
"source": [
"theta = Parameter(\"θ\")\n",
"\n",
"hop_gate = QuantumCircuit(2, name=\"Hop gate\")\n",
"hop_gate.h(0)\n",
"hop_gate.cx(1, 0)\n",
"hop_gate.cx(0, 1)\n",
"hop_gate.ry(-theta, 0)\n",
"hop_gate.ry(-theta, 1)\n",
"hop_gate.cx(0, 1)\n",
"hop_gate.h(0)\n",
"\n",
"hop_gate.draw()"
]
},
{
"cell_type": "code",
"execution_count": 4,
"metadata": {},
"outputs": [
{
"data": {
@@ -167,12 +108,23 @@
" └───────────────┘└──────────────┘└───────────────┘"
]
},
"execution_count": 4,
"execution_count": 3,
"metadata": {},
"output_type": "execute_result"
}
],
"source": [
"theta = Parameter(\"θ\")\n",
"\n",
"hop_gate = QuantumCircuit(2, name=\"Hop gate\")\n",
"hop_gate.h(0)\n",
"hop_gate.cx(1, 0)\n",
"hop_gate.cx(0, 1)\n",
"hop_gate.ry(-theta, 0)\n",
"hop_gate.ry(-theta, 1)\n",
"hop_gate.cx(0, 1)\n",
"hop_gate.h(0)\n",
"\n",
"theta_1, theta_2, theta_3, theta_4 = (\n",
" Parameter(\"θ1\"),\n",
" Parameter(\"θ2\"),\n",
@@ -197,6 +149,25 @@
"ansatz.circuit_u.draw()"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"Create the ``EntanglementForgingGroundStateSolver``, and specify which orbitals to reduce in the problem."
]
},
{
"cell_type": "code",
"execution_count": 4,
"metadata": {},
"outputs": [],
"source": [
"solver = EntanglementForgingGroundStateSolver(\n",
" ansatz=ansatz,\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": {},
@@ -212,7 +183,7 @@
{
"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'>Sun Jun 18 20:55:50 2023 CDT</td></tr></table>"
],
"text/plain": [
"<IPython.core.display.HTML object>"
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
``AttributeError``\ s when instantiating the `~circuit_knitting.forging.EntanglementForgingResult` in certain
conditions, such as when reducing the orbitals over which to solve.