Make objective direction checks more strict (#2382)

Summary:
Pull Request resolved: #2382

With this change, at least one of `minimize` or `lower_is_better` must be specified. If both are specified, they must match.

Added a json storage helper & updated SQA storage helpers for deserializing previously saved objectives in a backwards compatible manner, resolving the conflicts in favor of `minimize`.

Reviewed By: mpolson64

Differential Revision: D56315542

fbshipit-source-id: 5936fc3ecf5ee88ab80011ed095f98e097d16c21

Author: saitcakmak

ax/core/objective.py

@@ -13,6 +13,7 @@
 from typing import Any, Iterable, List, Optional, Tuple
 
 from ax.core.metric import Metric
+from ax.exceptions.core import UserInputError
 from ax.utils.common.base import SortableBase
 from ax.utils.common.logger import get_logger
 from ax.utils.common.typeutils import not_none
@@ -34,36 +35,27 @@ def __init__(self, metric: Metric, minimize: Optional[bool] = None) -> None:
             metric: The metric to be optimized.
             minimize: If True, minimize metric. If None, will be set based on the
                 `lower_is_better` property of the metric (if that is not specified,
-                will raise a DeprecationWarning).
+                will raise a `UserInputError`).
 
         """
         lower_is_better = metric.lower_is_better
         if minimize is None:
             if lower_is_better is None:
-                warnings.warn(
-                    f"Defaulting to `minimize=False` for metric {metric.name} not "
-                    + "specifying `lower_is_better` property. This is a wild guess. "
-                    + "Specify either `lower_is_better` on the metric, or specify "
-                    + "`minimize` explicitly. This will become an error in the future.",
-                    DeprecationWarning,
+                raise UserInputError(
+                    f"Metric {metric.name} does not specify `lower_is_better` "
+                    "and `minimize` is not specified. At least one of these "
+                    "must be specified."
                 )
-                minimize = False
             else:
                 minimize = lower_is_better
-        if lower_is_better is not None:
-            if lower_is_better and not minimize:
-                warnings.warn(
-                    f"Attempting to maximize metric {metric.name} with property "
-                    "`lower_is_better=True`."
-                )
-            elif not lower_is_better and minimize:
-                warnings.warn(
-                    f"Attempting to minimize metric {metric.name} with property "
-                    "`lower_is_better=False`."
-                )
-        self._metric = metric
-        # pyre-fixme[4]: Attribute must be annotated.
-        self.minimize = not_none(minimize)
+        elif lower_is_better is not None and lower_is_better != minimize:
+            raise UserInputError(
+                f"Metric {metric.name} specifies {lower_is_better=}, "
+                "which doesn't match the specified optimization direction "
+                f"{minimize=}."
+            )
+        self._metric: Metric = metric
+        self.minimize: bool = not_none(minimize)
 
     @property
     def metric(self) -> Metric:
@@ -130,18 +122,17 @@ def __init__(
                     "as input to `MultiObjective` constructor."
                 )
             metrics = extra_kwargs["metrics"]
-            minimize = extra_kwargs.get("minimize", False)
+            minimize = extra_kwargs.get("minimize", None)
             warnings.warn(
                 "Passing `metrics` and `minimize` as input to the `MultiObjective` "
                 "constructor will soon be deprecated. Instead, pass a list of "
                 "`objectives`. This will become an error in the future.",
                 DeprecationWarning,
+                stacklevel=2,
             )
             objectives = []
             for metric in metrics:
-                lower_is_better = metric.lower_is_better or False
-                _minimize = not lower_is_better if minimize else lower_is_better
-                objectives.append(Objective(metric=metric, minimize=_minimize))
+                objectives.append(Objective(metric=metric, minimize=minimize))
 
         # pyre-fixme[4]: Attribute must be annotated.
         self._objectives = not_none(objectives)

ax/core/tests/test_objective.py

@@ -8,6 +8,7 @@
 
 from ax.core.metric import Metric
 from ax.core.objective import MultiObjective, Objective, ScalarizedObjective
+from ax.exceptions.core import UserInputError
 from ax.utils.common.testutils import TestCase
 
 
@@ -20,7 +21,7 @@ def setUp(self) -> None:
             "m3": Metric(name="m3", lower_is_better=False),
         }
         self.objectives = {
-            "o1": Objective(metric=self.metrics["m1"]),
+            "o1": Objective(metric=self.metrics["m1"], minimize=True),
             "o2": Objective(metric=self.metrics["m2"], minimize=True),
             "o3": Objective(metric=self.metrics["m3"], minimize=False),
         }
@@ -37,6 +38,12 @@ def setUp(self) -> None:
         )
 
     def test_Init(self) -> None:
+        with self.assertRaisesRegex(UserInputError, "does not specify"):
+            Objective(metric=self.metrics["m1"]),
+        with self.assertRaisesRegex(
+            UserInputError, "doesn't match the specified optimization direction"
+        ):
+            Objective(metric=self.metrics["m2"], minimize=False)
         with self.assertRaises(ValueError):
             ScalarizedObjective(
                 metrics=[self.metrics["m1"], self.metrics["m2"]], weights=[1.0]
@@ -51,14 +58,6 @@ def test_Init(self) -> None:
                 metrics=[self.metrics["m1"], self.metrics["m2"]],
                 minimize=False,
             )
-        with self.assertWarnsRegex(
-            DeprecationWarning, "Defaulting to `minimize=False`"
-        ):
-            Objective(metric=self.metrics["m1"])
-        with self.assertWarnsRegex(UserWarning, "Attempting to maximize"):
-            Objective(Metric(name="m4", lower_is_better=True), minimize=False)
-        with self.assertWarnsRegex(UserWarning, "Attempting to minimize"):
-            Objective(Metric(name="m4", lower_is_better=False), minimize=True)
         self.assertEqual(
             self.objective.get_unconstrainable_metrics(), [self.metrics["m1"]]
         )
@@ -70,15 +69,15 @@ def test_MultiObjective(self) -> None:
 
         self.assertEqual(self.multi_objective.metrics, list(self.metrics.values()))
         minimizes = [obj.minimize for obj in self.multi_objective.objectives]
-        self.assertEqual(minimizes, [False, True, False])
+        self.assertEqual(minimizes, [True, True, False])
         weights = [mw[1] for mw in self.multi_objective.objective_weights]
         self.assertEqual(weights, [1.0, 1.0, 1.0])
         self.assertEqual(self.multi_objective.clone(), self.multi_objective)
         self.assertEqual(
             str(self.multi_objective),
             (
                 "MultiObjective(objectives="
-                '[Objective(metric_name="m1", minimize=False), '
+                '[Objective(metric_name="m1", minimize=True), '
                 'Objective(metric_name="m2", minimize=True), '
                 'Objective(metric_name="m3", minimize=False)])'
             ),
@@ -89,19 +88,26 @@ def test_MultiObjective(self) -> None:
         )
 
     def test_MultiObjectiveBackwardsCompatibility(self) -> None:
-        multi_objective = MultiObjective(
-            metrics=[self.metrics["m1"], self.metrics["m2"], self.metrics["m3"]]
-        )
+        metrics = [
+            Metric(name="m1", lower_is_better=False),
+            self.metrics["m2"],
+            self.metrics["m3"],
+        ]
+        multi_objective = MultiObjective(metrics=metrics)
         minimizes = [obj.minimize for obj in multi_objective.objectives]
-        self.assertEqual(multi_objective.metrics, list(self.metrics.values()))
+        self.assertEqual(multi_objective.metrics, metrics)
         self.assertEqual(minimizes, [False, True, False])
 
         multi_objective_min = MultiObjective(
-            metrics=[self.metrics["m1"], self.metrics["m2"], self.metrics["m3"]],
+            metrics=[
+                Metric(name="m1"),
+                Metric(name="m2"),
+                Metric(name="m3", lower_is_better=True),
+            ],
             minimize=True,
         )
         minimizes = [obj.minimize for obj in multi_objective_min.objectives]
-        self.assertEqual(minimizes, [True, False, True])
+        self.assertEqual(minimizes, [True, True, True])
 
     def test_ScalarizedObjective(self) -> None:
         with self.assertRaises(NotImplementedError):

ax/core/tests/test_optimization_config.py

@@ -279,7 +279,7 @@ def setUp(self) -> None:
             "o2": Objective(metric=self.metrics["m2"], minimize=False),
             "o3": Objective(metric=self.metrics["m3"], minimize=False),
         }
-        self.objective = Objective(metric=self.metrics["m1"], minimize=False)
+        self.objective = Objective(metric=self.metrics["m1"], minimize=True)
         self.multi_objective = MultiObjective(
             objectives=[self.objectives["o1"], self.objectives["o2"]]
         )

ax/core/tests/test_utils.py

@@ -158,7 +158,7 @@ def setUp(self) -> None:
         self.data = Data(df=self.df)
 
         self.optimization_config = OptimizationConfig(
-            objective=Objective(metric=Metric(name="a")),
+            objective=Objective(metric=Metric(name="a"), minimize=False),
             outcome_constraints=[
                 OutcomeConstraint(
                     metric=Metric(name="b"),

ax/modelbridge/tests/test_base_modelbridge.py

@@ -156,7 +156,7 @@ def test_ModelBridge(
                 observation_features=[get_observation1trans().features], weights=[2]
             ),
         )
-        oc = OptimizationConfig(objective=Objective(metric=Metric(name="test_metric")))
+        oc = get_optimization_config_no_constraints()
         modelbridge._set_kwargs_to_save(
             model_key="TestModel", model_kwargs={}, bridge_kwargs={}
         )
@@ -322,7 +322,7 @@ def warn_and_return_mock_obs(
             fit_tracking_metrics=False,
         )
         new_oc = OptimizationConfig(
-            objective=Objective(metric=Metric(name="test_metric2"))
+            objective=Objective(metric=Metric(name="test_metric2"), minimize=False),
         )
         with self.assertRaisesRegex(UnsupportedError, "fit_tracking_metrics"):
             modelbridge.gen(n=1, optimization_config=new_oc)

ax/modelbridge/tests/test_cross_validation.py

@@ -345,7 +345,7 @@ def test_HasGoodOptConfigModelFit(self) -> None:
 
         # Test single objective
         optimization_config = OptimizationConfig(
-            objective=Objective(metric=Metric("a"))
+            objective=Objective(metric=Metric("a"), minimize=True)
         )
         has_good_fit = has_good_opt_config_model_fit(
             optimization_config=optimization_config,
@@ -355,7 +355,12 @@ def test_HasGoodOptConfigModelFit(self) -> None:
 
         # Test multi objective
         optimization_config = MultiObjectiveOptimizationConfig(
-            objective=MultiObjective(metrics=[Metric("a"), Metric("b")])
+            objective=MultiObjective(
+                objectives=[
+                    Objective(Metric("a"), minimize=False),
+                    Objective(Metric("b"), minimize=False),
+                ]
+            )
         )
         has_good_fit = has_good_opt_config_model_fit(
             optimization_config=optimization_config,
@@ -365,7 +370,7 @@ def test_HasGoodOptConfigModelFit(self) -> None:
 
         # Test constraints
         optimization_config = OptimizationConfig(
-            objective=Objective(metric=Metric("a")),
+            objective=Objective(metric=Metric("a"), minimize=False),
             outcome_constraints=[
                 OutcomeConstraint(metric=Metric("b"), op=ComparisonOp.GEQ, bound=0.1)
             ],

ax/modelbridge/tests/test_torch_modelbridge.py

@@ -45,6 +45,7 @@
     get_branin_experiment,
     get_branin_search_space,
     get_experiment_with_observations,
+    get_optimization_config_no_constraints,
     get_search_space_for_range_value,
 )
 from ax.utils.testing.mock import fast_botorch_optimize
@@ -363,9 +364,7 @@ def test_evaluate_acquisition_function(self, _, mock_torch_model: Mock) -> None:
                 observation_features=[
                     ObservationFeatures(parameters={"x": 1.0, "y": 2.0})
                 ],
-                optimization_config=OptimizationConfig(
-                    objective=Objective(metric=Metric(name="test_metric"))
-                ),
+                optimization_config=get_optimization_config_no_constraints(),
             )
 
         self.assertEqual(acqf_vals, [5.0])
@@ -392,9 +391,7 @@ def test_evaluate_acquisition_function(self, _, mock_torch_model: Mock) -> None:
                     ObservationFeatures(parameters={"x": 1.0, "y": 2.0}),
                     ObservationFeatures(parameters={"x": 1.0, "y": 2.0}),
                 ],
-                optimization_config=OptimizationConfig(
-                    objective=Objective(metric=Metric(name="test_metric"))
-                ),
+                optimization_config=get_optimization_config_no_constraints(),
             )
         t.transform_observation_features.assert_any_call(
             [ObservationFeatures(parameters={"x": 1.0, "y": 2.0})],
@@ -418,9 +415,7 @@ def test_evaluate_acquisition_function(self, _, mock_torch_model: Mock) -> None:
                         ObservationFeatures(parameters={"x": 1.0, "y": 2.0}),
                     ]
                 ],
-                optimization_config=OptimizationConfig(
-                    objective=Objective(metric=Metric(name="test_metric"))
-                ),
+                optimization_config=get_optimization_config_no_constraints(),
             )
         t.transform_observation_features.assert_any_call(
             [

ax/modelbridge/tests/test_utils.py

@@ -118,7 +118,9 @@ def test_extract_outcome_constraints(self) -> None:
     def test_extract_objective_thresholds(self) -> None:
         outcomes = ["m1", "m2", "m3", "m4"]
         objective = MultiObjective(
-            objectives=[Objective(metric=Metric(name)) for name in outcomes[:3]]
+            objectives=[
+                Objective(metric=Metric(name), minimize=False) for name in outcomes[:3]
+            ]
         )
         objective_thresholds = [
             ObjectiveThreshold(
@@ -160,7 +162,7 @@ def test_extract_objective_thresholds(self) -> None:
         self.assertTrue(np.isnan(obj_t[-2:]).all())
 
         # Fails if a threshold does not have a corresponding metric.
-        objective2 = Objective(Metric("m1"))
+        objective2 = Objective(Metric("m1"), minimize=False)
         with self.assertRaisesRegex(ValueError, "corresponding metrics"):
             extract_objective_thresholds(
                 objective_thresholds=objective_thresholds,

ax/modelbridge/transforms/tests/test_derelativize_transform.py

@@ -103,7 +103,7 @@ def test_DerelativizeTransform(
         )
 
         # Test with no relative constraints
-        objective = Objective(Metric("c"))
+        objective = Objective(Metric("c"), minimize=True)
         oc = OptimizationConfig(
             objective=objective,
             outcome_constraints=[
@@ -301,7 +301,7 @@ def test_Errors(self) -> None:
             observations=[],
         )
         oc = OptimizationConfig(
-            objective=Objective(Metric("c")),
+            objective=Objective(Metric("c"), minimize=False),
             outcome_constraints=[
                 OutcomeConstraint(Metric("a"), ComparisonOp.LEQ, bound=2, relative=True)
             ],

ax/modelbridge/transforms/tests/test_winsorize_transform.py

@@ -578,7 +578,7 @@ def test_relative_constraints(
                 RangeParameter("y", ParameterType.FLOAT, 0, 20),
             ]
         )
-        objective = Objective(Metric("c"))
+        objective = Objective(Metric("c"), minimize=False)
 
         # Test with relative constraint, in-design status quo
         oc = OptimizationConfig(