BUG: can't call getVal from GenExpr (#1148)

* Add tests for expression evaluation in Model

Introduces the test_evaluate function to verify correct evaluation of various expressions using Model.getVal, including arithmetic and trigonometric operations, and checks for TypeError on invalid input.

* Add unified expression evaluation with _evaluate methods

Introduced _evaluate methods to Term, Expr, and all GenExpr subclasses for consistent evaluation of expressions and variables. Refactored Solution and Model to use these methods, simplifying and unifying value retrieval for variables and expressions. Cleaned up class definitions and improved hashing and equality for Term and Variable.

* Optimize matrix expression evaluation in Solution

Replaced explicit loop with flat iterator for evaluating matrix expressions in Solution.getVal, improving performance and code clarity.

* Add test for matrix variable evaluation

Introduces a new test 'test_evaluate' to verify correct evaluation of matrix variable division and summation in the model.

* Remove matrix expression handling in Solution __getitem__

Eliminated special handling for MatrixExpr and MatrixGenExpr in Solution.__getitem__, simplifying the method to only evaluate single expressions. This change streamlines the code and removes unnecessary numpy array construction.

* Refactor Term class variable naming and usage

Renamed 'vars' to 'vartuple' and '_hash' to 'hashval' in the Term class for clarity. Updated all references and methods to use the new attribute names, improving code readability and consistency.

* Use double precision for expression evaluation

Changed internal evaluation methods in expression classes from float to double for improved numerical precision. Updated type declarations and imports accordingly.

* Add type annotations and improve Term class attributes

Added type annotations to the Term constructor and specified types for class attributes. This improves code clarity and type safety.

* Make _evaluate methods public and refactor Expr/Variable

Changed all _evaluate methods in expression classes from cdef to cpdef to make them accessible from Python. Moved the definition of terms and _evaluate to the Expr class in scip.pxd, and refactored Variable to inherit from Expr in scip.pxd, consolidating class member definitions. These changes improve the interface and maintainability of expression evaluation.

* Update CHANGELOG.md

* Remove hash method from Variable

* back to old behavior

* Fix MatrixExpr _evaluate to return ndarray type

Ensures that MatrixExpr._evaluate returns a numpy ndarray when appropriate. Adds a test to verify the return type of getVal for matrix variables.

* Remove unused _evaluate method from Solution stub

Deleted the _evaluate method from the Solution class in the scip.pyi stub file, as it is no longer needed. Also added TYPE_CHECKING to the typing imports.

* Add @disjoint_base decorator to Term and UnaryExpr

Applied the @disjoint_base decorator to the Term and UnaryExpr classes in scip.pyi to clarify their base class relationships. This may improve type checking or class hierarchy handling.

* Add noqa to suppress unused import warning

Appended '# noqa: F401' to the 'ClassVar' import to suppress linter warnings about unused imports in scip.pyi.

* cache `_evaluate` function for matrix

* Refactor _evaluate to use np.frompyfunc

Replaces the @np.vectorize-decorated _evaluate function with an np.frompyfunc-based implementation for evaluating expressions with solutions. This change may improve compatibility and performance when applying _evaluate to arrays.

* Simplify _evaluate return in MatrixExpr

Refactored the _evaluate method in MatrixExpr to always return the result as a NumPy ndarray, removing the conditional type check.

* Expand test_evaluate with additional variable and cases

Added a second variable 'y' to test_evaluate and included new assertions for expressions involving both variables. This improves coverage of expression evaluation, including addition and division of variables.

* Update expected value in test_evaluate assertion

Changed the expected result of m.getVal(x + y + 1) from 3 to 4 in test_evaluate to reflect updated logic or correct the test expectation.

* Optimize Expr._evaluate by iterating dict with PyDict_Next

Replaces the standard Python dict iteration in Expr._evaluate with a more efficient C-level iteration using PyDict_Next. This change improves performance by avoiding Python-level overhead when evaluating expressions.

* Optimize Term evaluation with early exit on zero

Improved the _evaluate method in Term to return early if the product becomes zero, enhancing performance. Added a test case to verify correct evaluation when a variable is zero.

* Fix loop variable usage in Term evaluation

Replaces iteration over self.vartuple with a range-based loop to correctly access variables by index, preventing potential errors in variable assignment during term evaluation.

* Refactor variable names in Term class evaluation

Renamed local variables 'scip' and 'sol' to 'scip_ptr' and 'sol_ptr' in the Term class to improve clarity and avoid shadowing the input parameter 'sol'.

* Refactor variable initialization in _evaluate methods

Updated variable initialization in Term._evaluate and Expr._evaluate for clarity and consistency. Changed res initialization to 1.0 and consolidated variable declarations to improve code readability.

* Add exception specification to _evaluate methods

Appended 'except *' to all cpdef double _evaluate methods in expression classes to ensure proper exception handling in Cython. This change improves error propagation and consistency across the codebase.

* Fix _evaluate method calls in expression classes

Corrects the _evaluate method implementations in VarExpr, PowExpr, and UnaryExpr to explicitly cast children[0] to GenExpr before calling _evaluate. This ensures proper method resolution and avoids potential attribute errors.

* Optimize SumExpr and ProdExpr evaluation loops

Refactored the _evaluate methods in SumExpr and ProdExpr to use indexed loops for better performance and type safety. Added early exit in ProdExpr when result becomes zero to improve efficiency.

* Fix evaluation of UnaryExpr in expression module

Corrects the _evaluate method in UnaryExpr to properly evaluate the child expression before applying the math operation. This ensures that the math function receives a numeric value instead of an expression object.

* Fix error message grammar

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Replace _evaluate with _vec_evaluate in matrix.pxi

Removed the unused _evaluate function and replaced its usage with _vec_evaluate in the MatrixExpr class. This change clarifies the evaluation logic and ensures consistency in function naming.

* Change hashval type to Py_ssize_t in Term class

Updated the Term class in expr.pxi to use Py_ssize_t for the hashval attribute instead of int, ensuring compatibility with Python's hash function return type and improving type safety.

* Handle 'abs' operator in UnaryExpr evaluation

Special-cases the 'abs' operator in UnaryExpr._evaluate to use math.fabs, ensuring correct evaluation for absolute value expressions.

* Update getSolVal return type to support NDArray

Modified the getSolVal method in the Model class to return either a float or a numpy NDArray of float64, reflecting support for vectorized solution values. Also imported NDArray from numpy.typing to enable this type annotation.

* Expand getVal to support GenExpr in Model

Updated the getVal method in the Model class to accept GenExpr in addition to Expr and MatrixExpr. This broadens the method's applicability to more expression types.

* Fix type cast in VarExpr _evaluate method

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Remove unused include of matrix.pxi

The include statement for matrix.pxi was removed from expr.pxi as it is no longer needed.

* Update tests/test_expr.py

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: João Dionísio <57299939+Joao-Dionisio@users.noreply.github.com>

Author: 3 people

CHANGELOG.md

@@ -10,6 +10,7 @@
 - all fundamental callbacks now raise an error if not implemented
 - Fixed the type of MatrixExpr.sum(axis=...) result from MatrixVariable to MatrixExpr.
 - Updated IIS result in PyiisfinderExec()
+- Model.getVal now supports GenExpr type
 - Fixed lotsizing_lazy example
 - Fixed incorrect getVal() result when _bestSol.sol was outdated
 ### Changed

src/pyscipopt/expr.pxi

@@ -42,9 +42,20 @@
 # which should, in princple, modify the expr. However, since we do not implement __isub__, __sub__
 # gets called (I guess) and so a copy is returned.
 # Modifying the expression directly would be a bug, given that the expression might be re-used by the user. </pre>
+import math
+from typing import TYPE_CHECKING
+
+from pyscipopt.scip cimport Variable, Solution
+from cpython.dict cimport PyDict_Next
+from cpython.ref cimport PyObject
+
 import numpy as np
 
 
+if TYPE_CHECKING:
+    double = float
+
+
 def _is_number(e):
     try:
         f = float(e)
@@ -87,23 +98,25 @@ def _expr_richcmp(self, other, op):
         raise NotImplementedError("Can only support constraints with '<=', '>=', or '=='.")
 
 
-class Term:
+cdef class Term:
     '''This is a monomial term'''
 
-    __slots__ = ('vartuple', 'ptrtuple', 'hashval')
+    cdef readonly tuple vartuple
+    cdef readonly tuple ptrtuple
+    cdef Py_ssize_t hashval
 
-    def __init__(self, *vartuple):
+    def __init__(self, *vartuple: Variable):
         self.vartuple = tuple(sorted(vartuple, key=lambda v: v.ptr()))
         self.ptrtuple = tuple(v.ptr() for v in self.vartuple)
-        self.hashval = sum(self.ptrtuple)
+        self.hashval = <Py_ssize_t>hash(self.ptrtuple)
 
     def __getitem__(self, idx):
         return self.vartuple[idx]
 
-    def __hash__(self):
+    def __hash__(self) -> Py_ssize_t:
         return self.hashval
 
-    def __eq__(self, other):
+    def __eq__(self, other: Term):
         return self.ptrtuple == other.ptrtuple
 
     def __len__(self):
@@ -116,6 +129,20 @@ class Term:
     def __repr__(self):
         return 'Term(%s)' % ', '.join([str(v) for v in self.vartuple])
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        cdef double res = 1.0
+        cdef SCIP* scip_ptr = sol.scip
+        cdef SCIP_SOL* sol_ptr = sol.sol
+        cdef int i = 0, n = len(self)
+        cdef Variable var
+
+        for i in range(n):
+            var = <Variable>self.vartuple[i]
+            res *= SCIPgetSolVal(scip_ptr, sol_ptr, var.scip_var)
+            if res == 0:  # early stop
+                return 0.0
+        return res
+
 
 CONST = Term()
 
@@ -157,7 +184,7 @@ def buildGenExprObj(expr):
 ##@details Polynomial expressions of variables with operator overloading. \n
 #See also the @ref ExprDetails "description" in the expr.pxi. 
 cdef class Expr:
-    
+
     def __init__(self, terms=None):
         '''terms is a dict of variables to coefficients.
 
@@ -318,6 +345,20 @@ cdef class Expr:
         else:
             return max(len(v) for v in self.terms)
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        cdef double res = 0
+        cdef Py_ssize_t pos = <Py_ssize_t>0
+        cdef PyObject* key_ptr
+        cdef PyObject* val_ptr
+        cdef Term term
+        cdef double coef
+
+        while PyDict_Next(self.terms, &pos, &key_ptr, &val_ptr):
+            term = <Term>key_ptr
+            coef = <double>(<object>val_ptr)
+            res += coef * term._evaluate(sol)
+        return res
+
 
 cdef class ExprCons:
     '''Constraints with a polynomial expressions and lower/upper bounds.'''
@@ -427,10 +468,10 @@ Operator = Op()
 #
 #See also the @ref ExprDetails "description" in the expr.pxi. 
 cdef class GenExpr:
+
     cdef public _op
     cdef public children
 
-
     def __init__(self): # do we need it
         ''' '''
 
@@ -625,44 +666,88 @@ cdef class SumExpr(GenExpr):
     def __repr__(self):
         return self._op + "(" + str(self.constant) + "," + ",".join(map(lambda child : child.__repr__(), self.children)) + ")"
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        cdef double res = self.constant
+        cdef int i = 0, n = len(self.children)
+        cdef list children = self.children
+        cdef list coefs = self.coefs
+        for i in range(n):
+            res += <double>coefs[i] * (<GenExpr>children[i])._evaluate(sol)
+        return res
+
+
 # Prod Expressions
 cdef class ProdExpr(GenExpr):
+
     cdef public constant
+
     def __init__(self):
         self.constant = 1.0
         self.children = []
         self._op = Operator.prod
+
     def __repr__(self):
         return self._op + "(" + str(self.constant) + "," + ",".join(map(lambda child : child.__repr__(), self.children)) + ")"
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        cdef double res = self.constant
+        cdef list children = self.children
+        cdef int i = 0, n = len(children)
+        for i in range(n):
+            res *= (<GenExpr>children[i])._evaluate(sol)
+            if res == 0:  # early stop
+                return 0.0
+        return res
+
+
 # Var Expressions
 cdef class VarExpr(GenExpr):
+
     cdef public var
+
     def __init__(self, var):
         self.children = [var]
         self._op = Operator.varidx
+
     def __repr__(self):
         return self.children[0].__repr__()
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        return (<Expr>self.children[0])._evaluate(sol)
+
+
 # Pow Expressions
 cdef class PowExpr(GenExpr):
+
     cdef public expo
+
     def __init__(self):
         self.expo = 1.0
         self.children = []
         self._op = Operator.power
+
     def __repr__(self):
         return self._op + "(" + self.children[0].__repr__() + "," + str(self.expo) + ")"
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        return (<GenExpr>self.children[0])._evaluate(sol) ** self.expo
+
+
 # Exp, Log, Sqrt, Sin, Cos Expressions
 cdef class UnaryExpr(GenExpr):
     def __init__(self, op, expr):
         self.children = []
         self.children.append(expr)
         self._op = op
+
     def __repr__(self):
         return self._op + "(" + self.children[0].__repr__() + ")"
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        cdef double res = (<GenExpr>self.children[0])._evaluate(sol)
+        return math.fabs(res) if self._op == "abs" else getattr(math, self._op)(res)
+
+
 # class for constant expressions
 cdef class Constant(GenExpr):
     cdef public number
@@ -673,6 +758,10 @@ cdef class Constant(GenExpr):
     def __repr__(self):
         return str(self.number)
 
+    cpdef double _evaluate(self, Solution sol) except *:
+        return self.number
+
+
 def exp(expr):
     """returns expression with exp-function"""
     if isinstance(expr, MatrixExpr):   

src/pyscipopt/matrix.pxi

@@ -4,6 +4,7 @@
 """
 from typing import Literal, Optional, Tuple, Union
 import numpy as np
+from numpy.typing import NDArray
 try:
     # NumPy 2.x location
     from numpy.lib.array_utils import normalize_axis_tuple
@@ -12,6 +13,7 @@ except ImportError:
     from numpy.core.numeric import normalize_axis_tuple
 
 cimport numpy as cnp
+from pyscipopt.scip cimport Expr, Solution
 
 cnp.import_array()
 
@@ -142,6 +144,10 @@ class MatrixExpr(np.ndarray):
         return super().__rsub__(other).view(MatrixExpr)
 
 
+    def _evaluate(self, Solution sol) -> NDArray[np.float64]:
+        return _vec_evaluate(self, sol).view(np.ndarray)
+
+
 class MatrixGenExpr(MatrixExpr):
     pass
 
@@ -166,6 +172,9 @@ cdef inline _ensure_array(arg, bool convert_scalar = True):
     return np.array(arg, dtype=object) if convert_scalar else arg
 
 
+_vec_evaluate = np.frompyfunc(lambda expr, sol: expr._evaluate(sol), 2, 1)
+
+
 def _core_dot(cnp.ndarray a, cnp.ndarray b) -> Union[Expr, np.ndarray]:
     """
     Perform matrix multiplication between a N-Demension constant array and a N-Demension

src/pyscipopt/scip.pxd

@@ -2110,6 +2110,8 @@ cdef extern from "tpi/tpi.h":
 cdef class Expr:
     cdef public terms
 
+    cpdef double _evaluate(self, Solution sol)
+
 cdef class Event:
     cdef SCIP_EVENT* event
     # can be used to store problem data

src/pyscipopt/scip.pxi

@@ -21,6 +21,7 @@ from dataclasses import dataclass
 from typing import Union
 
 import numpy as np
+from numpy.typing import NDArray
 
 include "expr.pxi"
 include "lp.pxi"
@@ -1099,29 +1100,8 @@ cdef class Solution:
         return sol
 
     def __getitem__(self, expr: Union[Expr, MatrixExpr]):
-        if isinstance(expr, MatrixExpr):
-            result = np.zeros(expr.shape, dtype=np.float64)
-            for idx in np.ndindex(expr.shape):
-                result[idx] = self.__getitem__(expr[idx])
-            return result
-
-        # fast track for Variable
-        cdef SCIP_Real coeff
-        cdef _VarArray wrapper
-        if isinstance(expr, Variable):
-            wrapper = _VarArray(expr)
-            self._checkStage("SCIPgetSolVal")
-            return SCIPgetSolVal(self.scip, self.sol, wrapper.ptr[0])
-        return sum(self._evaluate(term)*coeff for term, coeff in expr.terms.items() if coeff != 0)
-
-    def _evaluate(self, term):
         self._checkStage("SCIPgetSolVal")
-        result = 1
-        cdef _VarArray wrapper
-        wrapper = _VarArray(term.vartuple)
-        for i in range(len(term.vartuple)):
-            result *= SCIPgetSolVal(self.scip, self.sol, wrapper.ptr[i])
-        return result
+        return expr._evaluate(self)
 
     def __setitem__(self, Variable var, value):
         PY_SCIP_CALL(SCIPsetSolVal(self.scip, self.sol, var.scip_var, value))
@@ -10747,7 +10727,11 @@ cdef class Model:
 
         return self.getSolObjVal(self._bestSol, original)
 
-    def getSolVal(self, Solution sol, Expr expr):
+    def getSolVal(
+        self,
+        Solution sol,
+        expr: Union[Expr, GenExpr],
+    ) -> Union[float, NDArray[np.float64]]:
         """
         Retrieve value of given variable or expression in the given solution or in
         the LP/pseudo solution if sol == None
@@ -10767,24 +10751,22 @@ cdef class Model:
         A variable is also an expression.
 
         """
+        if not isinstance(expr, (Expr, GenExpr)):
+            raise TypeError(
+                "Argument 'expr' has incorrect type (expected 'Expr' or 'GenExpr', "
+                f"got {type(expr)})"
+            )
         # no need to create a NULL solution wrapper in case we have a variable
-        cdef _VarArray wrapper
-        if sol == None and isinstance(expr, Variable):
-            wrapper = _VarArray(expr)
-            return SCIPgetSolVal(self._scip, NULL, wrapper.ptr[0])
-        if sol == None:
-            sol = Solution.create(self._scip, NULL)
-        return sol[expr]
+        return (sol or Solution.create(self._scip, NULL))[expr]
 
-    def getVal(self, expr: Union[Expr, MatrixExpr] ):
+    def getVal(self, expr: Union[Expr, GenExpr, MatrixExpr] ):
         """
         Retrieve the value of the given variable or expression in the best known solution.
         Can only be called after solving is completed.
 
         Parameters
         ----------
-        expr : Expr ot MatrixExpr
-            polynomial expression to query the value of
+        expr : Expr, GenExpr or MatrixExpr
 
         Returns
         -------

src/pyscipopt/scip.pyi

@@ -1,5 +1,5 @@
 from dataclasses import dataclass
-from typing import ClassVar
+from typing import TYPE_CHECKING, ClassVar  # noqa: F401
 
 import numpy
 from _typeshed import Incomplete
@@ -2062,7 +2062,6 @@ class Solution:
     data: Incomplete
     def __init__(self, *args: Incomplete, **kwargs: Incomplete) -> None: ...
     def _checkStage(self, method: Incomplete) -> Incomplete: ...
-    def _evaluate(self, term: Incomplete) -> Incomplete: ...
     def getOrigin(self) -> Incomplete: ...
     def retransform(self) -> Incomplete: ...
     def translate(self, target: Incomplete) -> Incomplete: ...
@@ -2122,6 +2121,7 @@ class SumExpr(GenExpr):
     constant: Incomplete
     def __init__(self, *args: Incomplete, **kwargs: Incomplete) -> None: ...
 
+@disjoint_base
 class Term:
     hashval: Incomplete
     ptrtuple: Incomplete
@@ -2138,6 +2138,7 @@ class Term:
     def __lt__(self, other: object) -> bool: ...
     def __ne__(self, other: object) -> bool: ...
 
+@disjoint_base
 class UnaryExpr(GenExpr):
     def __init__(self, *args: Incomplete, **kwargs: Incomplete) -> None: ...