Improve error messages

Author: staadecker

src/pyoframe/_core.py

@@ -5,7 +5,7 @@
 import warnings
 from abc import abstractmethod
 from collections.abc import Iterable, Mapping, Sequence
-from typing import TYPE_CHECKING, Literal, Union, overload
+from typing import TYPE_CHECKING, Literal, Self, Union, overload
 
 import pandas as pd
 import polars as pl
@@ -693,7 +693,7 @@ def __init__(self, data: pl.DataFrame, name: str | None = None):
             super().__init__(data, name=name)
 
     @classmethod
-    def constant(cls, constant: int | float) -> Expression:
+    def constant(cls, constant: int | float) -> Self:
         """Creates a new expression equal to the given constant.
 
         Examples:

src/pyoframe/_model.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import time
 from pathlib import Path
 from typing import TYPE_CHECKING, Any
 
@@ -92,6 +93,7 @@ class Model:
         "minimize",
         "maximize",
         "_logger",
+        "_last_log",
     ]
 
     def __init__(
@@ -119,13 +121,16 @@ def __init__(
         self._solver_uses_variable_names = solver_uses_variable_names
 
         self._logger = None
+        self._last_log = None
         if verbose:
             import logging
 
             self._logger = logging.getLogger("pyoframe")
             self._logger.addHandler(logging.NullHandler())
             self._logger.setLevel(logging.DEBUG)
 
+            self._last_log = time.time()
+
     @property
     def poi(self):
         """The underlying PyOptInterface model used to interact with the solver.
@@ -340,18 +345,6 @@ def objective(self) -> Objective:
         Raises:
             ValueError: If the objective has not been defined.
 
-        Examples:
-            >>> m = pf.Model()
-            >>> m.X = pf.Variable()
-            >>> m.objective
-            Traceback (most recent call last):
-            ...
-            ValueError: Objective is not defined.
-            >>> m.maximize = m.X
-            >>> m.objective
-            <Objective (linear) terms=1>
-            X
-
         See Also:
             [`Model.has_objective`][pyoframe.Model.has_objective]
         """
@@ -371,9 +364,11 @@ def objective(self, value: Operable):
         value._on_add_to_model(self, "objective")
 
     @property
-    def minimize(self) -> Objective | None:
+    def minimize(self) -> Objective:
         """Sets or gets the model's objective for minimization problems."""
-        if self.sense != ObjSense.MIN:
+        if self._objective is None:
+            raise ValueError("Objective is not defined.")
+        if self.sense == ObjSense.MAX:
             raise ValueError("Can't get .minimize in a maximization problem.")
         return self._objective
 
@@ -386,9 +381,11 @@ def minimize(self, value: Operable):
         self.objective = value
 
     @property
-    def maximize(self) -> Objective | None:
+    def maximize(self) -> Objective:
         """Sets or gets the model's objective for maximization problems."""
-        if self.sense != ObjSense.MAX:
+        if self._objective is None:
+            raise ValueError("Objective is not defined.")
+        if self.sense == ObjSense.MIN:
             raise ValueError("Can't get .maximize in a minimization problem.")
         return self._objective
 
@@ -414,30 +411,37 @@ def __setattr__(self, __name: str, __value: Any) -> None:
                     f"Cannot create {__name} since it was already created."
                 )
 
-            log = self._logger is not None
+            log = self._logger is not None and isinstance(
+                __value, (Constraint, Variable)
+            )
 
             if log:
-                import time
-
                 start_time = time.time()
 
             __value._on_add_to_model(self, __name)
 
+            if log:
+                solver_time = time.time() - start_time  # type: ignore
+
             if isinstance(__value, Variable):
                 self._variables.append(__value)
                 if self._var_map is not None:
                     self._var_map.add(__value)
-
                 if log:
-                    self._logger.debug(
-                        f"Added Variable '{__name}' (n={len(__value)}) to {self.solver.name} in {time.time() - start_time:.3g} seconds"
-                    )
+                    type_name = "variable"
             elif isinstance(__value, Constraint):
                 self._constraints.append(__value)
                 if log:
-                    self._logger.debug(
-                        f"Added Constraint '{__name}' (n={len(__value)}) to {self.solver.name} in {time.time() - start_time:.3g} seconds"
-                    )
+                    type_name = "constraint"
+
+            if log:
+                elapsed_time = time.time() - self._last_log  # type: ignore
+                self._last_log += elapsed_time  # type: ignore
+
+                self._logger.debug(  # type: ignore
+                    f"Added {type_name} '{__name}' to model ({elapsed_time:.1f}s elapsed, {solver_time:.1f}s for solver, n={len(__value)})"  # type: ignore
+                )
+
         return super().__setattr__(__name, __value)
 
     # Defining a custom __getattribute__ prevents type checkers from complaining about attribute access

tests/test_model.py

@@ -97,7 +97,7 @@ def test_verbose(default_solver, caplog):
     m = pf.Model(default_solver, verbose=True)
 
     m.X = pf.Variable(lb=0)
-    assert "Added Variable 'X'" in caplog.text
+    assert "Added variable 'X'" in caplog.text
 
     m.constr = m.X >= 5
-    assert "Added Constraint 'constr'" in caplog.text
+    assert "Added constraint 'constr'" in caplog.text

tests/test_objective.py

@@ -1,5 +1,7 @@
 """Tests for the Objective class."""
 
+import re
+
 import pytest
 
 import pyoframe as pf
@@ -20,3 +22,63 @@ def test_get_obj_value(solver):
     m.optimize()
 
     assert m.objective.value == pytest.approx(5)
+
+
+@pytest.mark.parametrize("obj", [5, pf.Expression.constant(10)])
+def test_creation(default_solver, obj):
+    m = pf.Model(solver=default_solver)
+
+    # cannot retrieve undefined objective
+    with pytest.raises(ValueError, match=re.escape("Objective is not defined.")):
+        m.objective
+
+    # Should throw error because missing sense
+    with pytest.raises(
+        ValueError,
+        match=re.escape(
+            "Can't set an objective without specifying the sense. Did you use .objective instead of .minimize or .maximize ?"
+        ),
+    ):
+        m.objective = obj
+
+    # This should work instead
+    m = pf.Model(solver=default_solver, sense="min")
+    m.objective = obj
+    assert m.sense == pf.ObjSense.MIN
+
+    # Cannot redefine objective
+    with pytest.raises(
+        ValueError,
+        match=re.escape("An objective already exists. Use += or -= to modify it."),
+    ):
+        m.objective = obj + obj
+
+    # But adding should work fine
+    m.objective += obj
+    m.objective -= obj
+
+    # Using minimize/maximize should also work
+    m = pf.Model(solver=default_solver)
+    m.minimize = obj
+    assert m.sense == pf.ObjSense.MIN
+
+    # Unless the sense keyword conflicts of course
+    m = pf.Model(solver=default_solver, sense="max")
+    with pytest.raises(
+        ValueError,
+        match=re.escape("Can't set .minimize in a maximization problem."),
+    ):
+        m.minimize = obj
+
+    # Modifying an unset objective should still be helpful.
+    m = pf.Model(solver=default_solver)
+    with pytest.raises(
+        ValueError,
+        match=re.escape("Objective is not defined."),
+    ):
+        m.objective += obj
+    with pytest.raises(
+        ValueError,
+        match=re.escape("Objective is not defined."),
+    ):
+        m.minimize += obj