From aec759f0fe295f62bfeafc0158b9b59891728023 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 19 Jul 2022 22:55:57 +0200 Subject: [PATCH 1/3] fix settings in optimizers --- .../algorithms/optimizers/scipy_optimizer.py | 21 +++++++++++++---- .../algorithms/optimizers/test_optimizers.py | 23 +++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/qiskit/algorithms/optimizers/scipy_optimizer.py b/qiskit/algorithms/optimizers/scipy_optimizer.py index d1a41111e378..37762f651ee4 100644 --- a/qiskit/algorithms/optimizers/scipy_optimizer.py +++ b/qiskit/algorithms/optimizers/scipy_optimizer.py @@ -86,11 +86,22 @@ def get_support_level(self): @property def settings(self) -> Dict[str, Any]: - settings = { - "max_evals_grouped": self._max_evals_grouped, - "options": self._options, - **self._kwargs, - } + if hasattr(self, "_OPTIONS"): + # all _OPTIONS should be keys in self._options, but add a failsafe here + attributes = [ + option + for option in self._OPTIONS # pylint: disable=no-member + if option in self._options.keys() + ] + + settings = {attr: self._options[attr] for attr in attributes} + else: + settings = {} + + settings["max_evals_grouped"] = self._max_evals_grouped + settings["options"] = self._options.copy() + settings.update(self._kwargs) + # the subclasses don't need the "method" key as the class type specifies the method if self.__class__ == SciPyOptimizer: settings["method"] = self._method diff --git a/test/python/algorithms/optimizers/test_optimizers.py b/test/python/algorithms/optimizers/test_optimizers.py index 0bfe1d14cdde..e9003e9db346 100644 --- a/test/python/algorithms/optimizers/test_optimizers.py +++ b/test/python/algorithms/optimizers/test_optimizers.py @@ -226,6 +226,29 @@ def test_scipy(self, method, options): self.assertEqual(from_dict._method, method.lower()) self.assertEqual(from_dict._options, options) + def test_independent_reconstruction(self): + """Test the SciPyOptimizers don't reset all settings upon creating a new instance. + + COBYLA is used as representative example here.""" + + kwargs = {"coffee": "without sugar"} + options = {"tea": "with milk"} + optimizer = COBYLA(maxiter=1, options=options, **kwargs) + serialized = optimizer.settings + from_dict = COBYLA(**serialized) + + with self.subTest(msg="test attributes"): + self.assertEqual(from_dict.settings["options"]["maxiter"], 1) + + with self.subTest(msg="test options"): + self.assertEqual(from_dict.settings["options"]["tea"], "with milk") + + with self.subTest(msg="test kwargs"): + self.assertEqual(from_dict.settings["coffee"], "without sugar") + + with self.subTest(msg="option ids differ"): + self.assertNotEqual(id(serialized["options"]), id(from_dict.settings["options"])) + def test_adam(self): """Test ADAM is serializable.""" From 66778d430441ada71bac318834c8320c061197b4 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 19 Jul 2022 23:01:30 +0200 Subject: [PATCH 2/3] add reno --- .../fix-optimizer-settings-881585bfa8130cb7.yaml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 releasenotes/notes/fix-optimizer-settings-881585bfa8130cb7.yaml diff --git a/releasenotes/notes/fix-optimizer-settings-881585bfa8130cb7.yaml b/releasenotes/notes/fix-optimizer-settings-881585bfa8130cb7.yaml new file mode 100644 index 000000000000..ca0b42f965a6 --- /dev/null +++ b/releasenotes/notes/fix-optimizer-settings-881585bfa8130cb7.yaml @@ -0,0 +1,15 @@ +--- +fixes: + - | + Fix a bug in the :class:`~.Optimizer` classes where re-constructing a new optimizer instance + from a previously exisiting :attr:`~.Optimizer.settings` reset both the new and previous + optimizer settings to the defaults. This notably led to a bug if :class:`~.Optimizer` objects + were send as input to Qiskit Runtime programs. + + Now optimizer objects are correctly reconstructed:: + + >>> from qiskit.algorithms.optimizers import COBYLA + >>> original = COBYLA(maxiter=1) + >>> reconstructed = COBYLA(**original.settings) + >>> reconstructed._options["maxiter"] + 1 # used to be 1000! From e1e06c938e59292e437faad3a312eda8061b9cc6 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 19 Jul 2022 23:10:37 +0200 Subject: [PATCH 3/3] don't duplicate args --- qiskit/algorithms/optimizers/scipy_optimizer.py | 7 ++++--- test/python/algorithms/optimizers/test_optimizers.py | 6 ++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/qiskit/algorithms/optimizers/scipy_optimizer.py b/qiskit/algorithms/optimizers/scipy_optimizer.py index 37762f651ee4..aa8349d03f33 100644 --- a/qiskit/algorithms/optimizers/scipy_optimizer.py +++ b/qiskit/algorithms/optimizers/scipy_optimizer.py @@ -86,20 +86,21 @@ def get_support_level(self): @property def settings(self) -> Dict[str, Any]: + options = self._options.copy() if hasattr(self, "_OPTIONS"): # all _OPTIONS should be keys in self._options, but add a failsafe here attributes = [ option for option in self._OPTIONS # pylint: disable=no-member - if option in self._options.keys() + if option in options.keys() ] - settings = {attr: self._options[attr] for attr in attributes} + settings = {attr: options.pop(attr) for attr in attributes} else: settings = {} settings["max_evals_grouped"] = self._max_evals_grouped - settings["options"] = self._options.copy() + settings["options"] = options settings.update(self._kwargs) # the subclasses don't need the "method" key as the class type specifies the method diff --git a/test/python/algorithms/optimizers/test_optimizers.py b/test/python/algorithms/optimizers/test_optimizers.py index e9003e9db346..5ce8e87cb1b6 100644 --- a/test/python/algorithms/optimizers/test_optimizers.py +++ b/test/python/algorithms/optimizers/test_optimizers.py @@ -238,10 +238,12 @@ def test_independent_reconstruction(self): from_dict = COBYLA(**serialized) with self.subTest(msg="test attributes"): - self.assertEqual(from_dict.settings["options"]["maxiter"], 1) + self.assertEqual(from_dict.settings["maxiter"], 1) with self.subTest(msg="test options"): - self.assertEqual(from_dict.settings["options"]["tea"], "with milk") + # options should only contain values that are *not* already in the initializer + # (e.g. should not contain maxiter) + self.assertEqual(from_dict.settings["options"], {"tea": "with milk"}) with self.subTest(msg="test kwargs"): self.assertEqual(from_dict.settings["coffee"], "without sugar")