Merge pull request #619 from stan-dev/fix/617-tol-params

Tighten optimization argument requirements, print command

Author: WardBrian

cmdstanpy/cmdstan_args.py

@@ -396,7 +396,7 @@ def __init__(
         history_size: Optional[int] = None,
     ) -> None:
 
-        self.algorithm = algorithm
+        self.algorithm = algorithm or ""
         self.init_alpha = init_alpha
         self.iter = iter
         self.save_iterations = save_iterations
@@ -414,20 +414,17 @@ def validate(
         """
         Check arguments correctness and consistency.
         """
-        if (
-            self.algorithm is not None
-            and self.algorithm not in self.OPTIMIZE_ALGOS
-        ):
+        if self.algorithm and self.algorithm not in self.OPTIMIZE_ALGOS:
             raise ValueError(
                 'Please specify optimizer algorithms as one of [{}]'.format(
                     ', '.join(self.OPTIMIZE_ALGOS)
                 )
             )
 
         if self.init_alpha is not None:
-            if self.algorithm == 'Newton':
+            if self.algorithm.lower() not in {'lbfgs', 'bfgs'}:
                 raise ValueError(
-                    'init_alpha must not be set when algorithm is Newton'
+                    'init_alpha requires that algorithm be set to bfgs or lbfgs'
                 )
             if isinstance(self.init_alpha, float):
                 if self.init_alpha <= 0:
@@ -443,9 +440,9 @@ def validate(
                 raise ValueError('iter must be type of int')
 
         if self.tol_obj is not None:
-            if self.algorithm == 'Newton':
+            if self.algorithm.lower() not in {'lbfgs', 'bfgs'}:
                 raise ValueError(
-                    'tol_obj must not be set when algorithm is Newton'
+                    'tol_obj requires that algorithm be set to bfgs or lbfgs'
                 )
             if isinstance(self.tol_obj, float):
                 if self.tol_obj <= 0:
@@ -454,9 +451,10 @@ def validate(
                 raise ValueError('tol_obj must be type of float')
 
         if self.tol_rel_obj is not None:
-            if self.algorithm == 'Newton':
+            if self.algorithm.lower() not in {'lbfgs', 'bfgs'}:
                 raise ValueError(
-                    'tol_rel_obj must not be set when algorithm is Newton'
+                    'tol_rel_obj requires that algorithm be set to bfgs'
+                    ' or lbfgs'
                 )
             if isinstance(self.tol_rel_obj, float):
                 if self.tol_rel_obj <= 0:
@@ -465,9 +463,9 @@ def validate(
                 raise ValueError('tol_rel_obj must be type of float')
 
         if self.tol_grad is not None:
-            if self.algorithm == 'Newton':
+            if self.algorithm.lower() not in {'lbfgs', 'bfgs'}:
                 raise ValueError(
-                    'tol_grad must not be set when algorithm is Newton'
+                    'tol_grad requires that algorithm be set to bfgs or lbfgs'
                 )
             if isinstance(self.tol_grad, float):
                 if self.tol_grad <= 0:
@@ -476,9 +474,10 @@ def validate(
                 raise ValueError('tol_grad must be type of float')
 
         if self.tol_rel_grad is not None:
-            if self.algorithm == 'Newton':
+            if self.algorithm.lower() not in {'lbfgs', 'bfgs'}:
                 raise ValueError(
-                    'tol_rel_grad must not be set when algorithm is Newton'
+                    'tol_rel_grad requires that algorithm be set to bfgs'
+                    ' or lbfgs'
                 )
             if isinstance(self.tol_rel_grad, float):
                 if self.tol_rel_grad <= 0:
@@ -487,9 +486,9 @@ def validate(
                 raise ValueError('tol_rel_grad must be type of float')
 
         if self.tol_param is not None:
-            if self.algorithm == 'Newton':
+            if self.algorithm.lower() not in {'lbfgs', 'bfgs'}:
                 raise ValueError(
-                    'tol_param must not be set when algorithm is Newton'
+                    'tol_param requires that algorithm be set to bfgs or lbfgs'
                 )
             if isinstance(self.tol_param, float):
                 if self.tol_param <= 0:
@@ -498,10 +497,9 @@ def validate(
                 raise ValueError('tol_param must be type of float')
 
         if self.history_size is not None:
-            if self.algorithm == 'Newton' or self.algorithm == 'BFGS':
+            if self.algorithm.lower() != 'lbfgs':
                 raise ValueError(
-                    'history_size must not be set when algorithm is '
-                    'Newton or BFGS'
+                    'history_size requires that algorithm be set to lbfgs'
                 )
             if isinstance(self.history_size, int):
                 if self.history_size < 0:

cmdstanpy/model.py

@@ -701,7 +701,9 @@ def optimize(
             self._run_cmdstan(runset, dummy_chain_id, show_console=show_console)
 
         if not runset._check_retcodes():
-            msg = 'Error during optimization: {}'.format(runset.get_err_msgs())
+            msg = "Error during optimization! Command '{}' failed: {}".format(
+                ' '.join(runset.cmd(0)), runset.get_err_msgs()
+            )
             if 'Line search failed' in msg and not require_converged:
                 get_logger().warning(msg)
             else:

test/test_cmdstan_args.py

@@ -34,11 +34,14 @@ def test_args_algorithm(self):
         self.assertIn('algorithm=newton', ' '.join(cmd))
 
     def test_args_algorithm_init_alpha(self):
-        args = OptimizeArgs(init_alpha=2e-4)
+        args = OptimizeArgs(algorithm='bfgs', init_alpha=2e-4)
         args.validate()
         cmd = args.compose(None, cmd=['output'])
 
         self.assertIn('init_alpha=0.0002', ' '.join(cmd))
+        args = OptimizeArgs(init_alpha=2e-4)
+        with self.assertRaises(ValueError):
+            args.validate()
         args = OptimizeArgs(init_alpha=-1.0)
         with self.assertRaises(ValueError):
             args.validate()

test/test_optimize.py

@@ -432,9 +432,7 @@ def test_parameters_and_optimizer_compatible(self):
         jdata = os.path.join(DATAFILES_PATH, 'bernoulli.data.json')
         jinit = os.path.join(DATAFILES_PATH, 'bernoulli.init.json')
 
-        with self.assertRaisesRegex(
-            ValueError, 'must not be set when algorithm is Newton'
-        ):
+        with self.assertRaisesRegex(ValueError, 'bfgs or lbfgs'):
             model.optimize(
                 data=jdata,
                 seed=1239812093,
@@ -443,9 +441,7 @@ def test_parameters_and_optimizer_compatible(self):
                 tol_obj=1,
             )
 
-        with self.assertRaisesRegex(
-            ValueError, 'must not be set when algorithm is Newton'
-        ):
+        with self.assertRaisesRegex(ValueError, 'bfgs or lbfgs'):
             model.optimize(
                 data=jdata,
                 seed=1239812093,
@@ -454,9 +450,7 @@ def test_parameters_and_optimizer_compatible(self):
                 tol_rel_obj=1,
             )
 
-        with self.assertRaisesRegex(
-            ValueError, 'must not be set when algorithm is Newton'
-        ):
+        with self.assertRaisesRegex(ValueError, 'bfgs or lbfgs'):
             model.optimize(
                 data=jdata,
                 seed=1239812093,
@@ -465,9 +459,7 @@ def test_parameters_and_optimizer_compatible(self):
                 tol_grad=1,
             )
 
-        with self.assertRaisesRegex(
-            ValueError, 'must not be set when algorithm is Newton'
-        ):
+        with self.assertRaisesRegex(ValueError, 'bfgs or lbfgs'):
             model.optimize(
                 data=jdata,
                 seed=1239812093,
@@ -476,9 +468,15 @@ def test_parameters_and_optimizer_compatible(self):
                 tol_rel_grad=1,
             )
 
-        with self.assertRaisesRegex(
-            ValueError, 'must not be set when algorithm is Newton'
-        ):
+        with self.assertRaisesRegex(ValueError, 'bfgs or lbfgs'):
+            model.optimize(
+                data=jdata,
+                seed=1239812093,
+                inits=jinit,
+                tol_rel_grad=1,
+            )
+
+        with self.assertRaisesRegex(ValueError, 'bfgs or lbfgs'):
             model.optimize(
                 data=jdata,
                 seed=1239812093,
@@ -489,7 +487,7 @@ def test_parameters_and_optimizer_compatible(self):
 
         with self.assertRaisesRegex(
             ValueError,
-            'history_size must not be set when algorithm is Newton or BFGS',
+            'lbfgs',
         ):
             model.optimize(
                 data=jdata,
@@ -501,7 +499,7 @@ def test_parameters_and_optimizer_compatible(self):
 
         with self.assertRaisesRegex(
             ValueError,
-            'history_size must not be set when algorithm is Newton or BFGS',
+            'lbfgs',
         ):
             model.optimize(
                 data=jdata,
@@ -511,6 +509,17 @@ def test_parameters_and_optimizer_compatible(self):
                 history_size=1,
             )
 
+        with self.assertRaisesRegex(
+            ValueError,
+            'lbfgs',
+        ):
+            model.optimize(
+                data=jdata,
+                seed=1239812093,
+                inits=jinit,
+                history_size=1,
+            )
+
     def test_optimize_good_dict(self):
         exe = os.path.join(DATAFILES_PATH, 'bernoulli' + EXTENSION)
         stan = os.path.join(DATAFILES_PATH, 'bernoulli.stan')