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

Bug fix on QNSPSA #9483

Merged
merged 13 commits into from
Feb 2, 2023
5 changes: 3 additions & 2 deletions qiskit/algorithms/optimizers/qnspsa.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,10 @@ def _point_sample(self, loss, x, eps, delta1, delta2):
gradient_estimate = (loss_values[0] - loss_values[1]) / (2 * eps) * delta1

# compute the preconditioner point estimate
fidelity_values = [float(f) for f in fidelity_values]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the casts to float still required now that we explicitly use diff = diff / thing instead of diff /= thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so because it failed again with diff = diff / thing with the same error

diff = fidelity_values[2] - fidelity_values[0]
diff -= fidelity_values[3] - fidelity_values[1]
diff /= 2 * eps**2
diff = diff - (fidelity_values[3] - fidelity_values[1])
diff = diff / (2 * float(eps)**2)

rank_one = np.outer(delta1, delta2)
# -0.5 factor comes from the fact that we need -0.5 * fidelity
Expand Down
5 changes: 5 additions & 0 deletions releasenotes/notes/qnspsa-float-bug-fix-4035f7e1eb61dec2.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
fixes:
- |
Fixed bug in QNSPSA which raised a type error when trying to divide or subtract
an int by a float or viceversa.
luciacuervovalor marked this conversation as resolved.
Show resolved Hide resolved
23 changes: 23 additions & 0 deletions test/python/algorithms/optimizers/test_spsa.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,3 +255,26 @@ def objective(x):
with self.subTest("check number of function calls"):
expected_nfev = 8 # 7 * maxiter + 1
self.assertEqual(result.nfev, expected_nfev)

def test_point_sample(self):
"""Test point sample function in QNSPSA"""

def fidelity(x, y):
Copy link
Member

Choose a reason for hiding this comment

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

pylint's moaning about the unused y argument. Given this is just a stub function, you can make it shut up by giving the second argument a name starting with an underscore, like _y - that means "explicitly ignored".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Oh man, and now Pylint's complaining that that's not a valid name... I'm not a fan of these pylint rules at all.

At any rate, you can put a comment just under the docstring of test_point_sample that says # pylint: disable=invalid-name or # pylint: disable=unused-argument as appropriate (depending on which name for the argument you prefer using).

x = np.asarray(x)
return np.ones_like(x, dtype=float) # some float

def objective(x):
return x

def get_perturbation():
def perturbation():
while True:
yield 1
return perturbation

qnspsa = QNSPSA(fidelity, maxiter=1, learning_rate=0.1, perturbation=get_perturbation())
initial_point = 1.
result = qnspsa.minimize(objective, initial_point)

expected_nfev = 8 # 7 * maxiter + 1
self.assertEqual(result.nfev, expected_nfev)