BUG: can't call `getVal` from `GenExpr` #1148

Zeroto521 · 2026-01-08T12:31:11Z

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.

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.

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

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

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.

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.

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

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

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.

src/pyscipopt/expr.pxi

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

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.

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.

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

src/pyscipopt/matrix.pxi

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.

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

codecov · 2026-01-09T11:48:43Z

Codecov Report

❌ Patch coverage is 62.79070% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.12%. Comparing base (5ceac82) to head (7cd196a).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/pyscipopt/expr.pxi	62.50%	12 Missing ⚠️
src/pyscipopt/matrix.pxi	25.00%	3 Missing ⚠️
src/pyscipopt/scip.pxi	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   55.07%   55.12%   +0.04%     
==========================================
  Files          24       24              
  Lines        5420     5448      +28     
==========================================
+ Hits         2985     3003      +18     
- Misses       2435     2445      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tests/test_expr.py

src/pyscipopt/expr.pxi

src/pyscipopt/matrix.pxi

src/pyscipopt/expr.pxi

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.

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.

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.

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'.

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.

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.

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.

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.

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.

Zeroto521 · 2026-01-19T05:07:25Z

Use Cython code style to speed up _evaluate. For example, use PyDict_Next to iterate over dict, which could avoid creating temporary Python objects.

Joao-Dionisio · 2026-01-20T09:59:05Z

This is what another AI had to say about the PR, can you please address or refute the comments?

  Issues/Concerns                                                                                                                                                                                               
                                                                                                                                                                                                                
  1. Hash function change (potential breaking change):                                                                                                                                                          
  # Before:                                                                                                                                                                                                     
  self.hashval = sum(self.ptrtuple)                                                                                                                                                                             
  # After:                                                                                                                                                                                                      
  self.hashval = hash(self.ptrtuple)                                                                                                                                                                            
  1. While hash(self.ptrtuple) is arguably better (avoids collisions for terms with same sum of pointers), this changes the hash values of existing Term objects. Any code that pickled/stored hash values would
   break. This should be documented in CHANGELOG as a breaking change or reverted if backward compatibility is needed.                                                                                          
  2. Missing _evaluate declaration in scip.pxd for GenExpr and its subclasses. Only Expr._evaluate is declared:                                                                                                 
  cdef class Expr:                                                                                                                                                                                              
      cdef public terms                                                                                                                                                                                         
      cpdef double _evaluate(self, Solution sol)                                                                                                                                                                
  2. The GenExpr classes should also have their _evaluate declared in .pxd for type safety.                                                                                                                     
  3. Module-level _evaluate in matrix.pxi:                                                                                                                                                                      
  _evaluate = np.frompyfunc(lambda expr, sol: expr._evaluate(sol), 2, 1)                                                                                                                                        
  3. This creates a module-level variable that shadows the method name. Consider renaming to _matrix_evaluate or similar.                                                                                                                                                                                                                                           
  4. Type annotation change in getSolVal:                                                                                                                                                                       
  def getSolVal(self, Solution sol, expr: Union[Expr, GenExpr]) -> float:                                                                                                                                       
  4. The return type is float, but for MatrixExpr it returns NDArray. This should be Union[float, NDArray] or keep it untyped.                                                                                  
  5. The if TYPE_CHECKING: double = float pattern is unconventional. It's used to avoid IDE warnings but could confuse readers.                                                                                 
                                                                                                                                                                                                                
  Minor Suggestions                                                                                                                                                                                             
                                                                                                                                                                                                                
  1. The @disjoint_base decorator added to Term and UnaryExpr in stubs - what's the purpose? This should be documented.                                                                                         
  2. Consider adding a docstring to the _evaluate methods explaining what they do.

Copilot

Pull request overview

This pull request fixes issue #1062 by adding support for evaluating GenExpr (general expression) types when calling Model.getVal() or Model.getSolVal(). Previously, only Expr (polynomial expression) types were supported, which caused TypeError when trying to evaluate expressions containing division operations or other non-polynomial operations.

Changes:

Added _evaluate() methods to all GenExpr subclasses (SumExpr, ProdExpr, VarExpr, PowExpr, UnaryExpr, Constant) and Expr/Term classes to support expression evaluation
Refactored Solution.__getitem__ to delegate evaluation to expression objects via their _evaluate() methods
Updated Model.getSolVal() to accept both Expr and GenExpr types

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/pyscipopt/expr.pxi	Added `_evaluate()` implementation for `Term`, `Expr`, and all `GenExpr` subclasses; converted `Term` from Python class to Cython `cdef class`; improved `Term` hash function
src/pyscipopt/scip.pxi	Simplified `Solution.__getitem__` to call `expr._evaluate()`; updated `Model.getSolVal()` to accept `GenExpr` types
src/pyscipopt/scip.pxd	Added `cpdef _evaluate` declaration to `Expr` class
src/pyscipopt/scip.pyi	Added `@disjoint_base` decorators to `Term` and `UnaryExpr`; removed obsolete `_evaluate` method from `Solution` stub
src/pyscipopt/matrix.pxi	Added `_evaluate()` method to `MatrixExpr` using numpy universal function
tests/test_expr.py	Added comprehensive tests for expression evaluation including division, power, and trigonometric functions
tests/test_matrix_variable.py	Added basic test for matrix variable evaluation
CHANGELOG.md	Documented the fix for GenExpr support

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/pyscipopt/expr.pxi

CHANGELOG.md

src/pyscipopt/expr.pxi

src/pyscipopt/scip.pxi

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

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.

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.

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

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.

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.

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

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

…nto issue/1062

src/pyscipopt/expr.pxi

Zeroto521 · 2026-01-21T05:53:19Z

Except for hash(self.ptrtuple) (#1148 (comment)), others are fixed.

This is what another AI had to say about the PR, can you please address or refute the comments?

  Issues/Concerns                                                                                                                                                                                               
                                                                                                                                                                                                                
  1. Hash function change (potential breaking change):                                                                                                                                                          
  # Before:                                                                                                                                                                                                     
  self.hashval = sum(self.ptrtuple)                                                                                                                                                                             
  # After:                                                                                                                                                                                                      
  self.hashval = hash(self.ptrtuple)                                                                                                                                                                            
  1. While hash(self.ptrtuple) is arguably better (avoids collisions for terms with same sum of pointers), this changes the hash values of existing Term objects. Any code that pickled/stored hash values would
   break. This should be documented in CHANGELOG as a breaking change or reverted if backward compatibility is needed.                                                                                          
  2. Missing _evaluate declaration in scip.pxd for GenExpr and its subclasses. Only Expr._evaluate is declared:                                                                                                 
  cdef class Expr:                                                                                                                                                                                              
      cdef public terms                                                                                                                                                                                         
      cpdef double _evaluate(self, Solution sol)                                                                                                                                                                
  2. The GenExpr classes should also have their _evaluate declared in .pxd for type safety.                                                                                                                     
  3. Module-level _evaluate in matrix.pxi:                                                                                                                                                                      
  _evaluate = np.frompyfunc(lambda expr, sol: expr._evaluate(sol), 2, 1)                                                                                                                                        
  3. This creates a module-level variable that shadows the method name. Consider renaming to _matrix_evaluate or similar.                                                                                                                                                                                                                                           
  4. Type annotation change in getSolVal:                                                                                                                                                                       
  def getSolVal(self, Solution sol, expr: Union[Expr, GenExpr]) -> float:                                                                                                                                       
  4. The return type is float, but for MatrixExpr it returns NDArray. This should be Union[float, NDArray] or keep it untyped.                                                                                  
  5. The if TYPE_CHECKING: double = float pattern is unconventional. It's used to avoid IDE warnings but could confuse readers.                                                                                 
                                                                                                                                                                                                                
  Minor Suggestions                                                                                                                                                                                             
                                                                                                                                                                                                                
  1. The @disjoint_base decorator added to Term and UnaryExpr in stubs - what's the purpose? This should be documented.                                                                                         
  2. Consider adding a docstring to the _evaluate methods explaining what they do.

tests/test_expr.py

CHANGELOG.md

src/pyscipopt/expr.pxi

Zeroto521 added 10 commits January 7, 2026 22:41

Add tests for expression evaluation in Model

f87432c

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.

Optimize matrix expression evaluation in Solution

85ee97e

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

Add test for matrix variable evaluation

135b954

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__

ad7d4c6

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

5ff4144

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

95b3a80

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

7770a06

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

Update CHANGELOG.md

239c3a3

Zeroto521 commented Jan 8, 2026

View reviewed changes

src/pyscipopt/expr.pxi Show resolved Hide resolved

Zeroto521 commented Jan 8, 2026

View reviewed changes

src/pyscipopt/expr.pxi Show resolved Hide resolved

Zeroto521 commented Jan 8, 2026

View reviewed changes

src/pyscipopt/expr.pxi Show resolved Hide resolved

Zeroto521 added 6 commits January 8, 2026 20:46

Remove hash method from Variable

d2ab8e2

back to old behavior

2b145dd

Fix MatrixExpr _evaluate to return ndarray type

9afb07f

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

66c2f6b

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

4ff2682

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

073ac1c

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

Zeroto521 commented Jan 8, 2026

View reviewed changes

src/pyscipopt/matrix.pxi Show resolved Hide resolved

Zeroto521 added 3 commits January 9, 2026 17:32

cache _evaluate function for matrix

22b9f4f

Refactor _evaluate to use np.frompyfunc

449e2fd

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

7cd196a

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

Merge branch 'master' into issue/1062

3ff4658

Joao-Dionisio reviewed Jan 17, 2026

View reviewed changes

src/pyscipopt/expr.pxi Show resolved Hide resolved

Zeroto521 added 3 commits January 19, 2026 10:43

Expand test_evaluate with additional variable and cases

aee875a

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

6470585

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

93d8b1e

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.

Zeroto521 added 6 commits January 19, 2026 11:07

Refactor variable names in Term class evaluation

2007671

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

0b41acd

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

75956fd

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

33fea88

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

cc140b4

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

6b56c88

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.

Zeroto521 requested a review from Joao-Dionisio January 19, 2026 05:16

Joao-Dionisio requested a review from Copilot January 20, 2026 09:53

Copilot started reviewing on behalf of Joao-Dionisio January 20, 2026 09:53 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

Joao-Dionisio mentioned this pull request Jan 20, 2026

BUG: can't call getVal from a matrix expression #1062

Closed

Zeroto521 and others added 10 commits January 21, 2026 13:11

Merge branch 'master' into issue/1062

cc9f254

Fix error message grammar

9f48a61

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

Replace _evaluate with _vec_evaluate in matrix.pxi

c91a78b

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

baf50b2

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

db7ea72

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

57f906a

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

8c45b7d

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

03cf742

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

Remove unused include of matrix.pxi

d3121b1

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

Merge branch 'issue/1062' of https://github.com/Zeroto521/PySCIPOpt i…

10d88d9

…nto issue/1062

Zeroto521 commented Jan 21, 2026

View reviewed changes

src/pyscipopt/expr.pxi Show resolved Hide resolved

Joao-Dionisio approved these changes Jan 21, 2026

View reviewed changes

tests/test_expr.py Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

src/pyscipopt/expr.pxi Show resolved Hide resolved

src/pyscipopt/expr.pxi Show resolved Hide resolved

Update tests/test_expr.py

e441513

Joao-Dionisio enabled auto-merge (squash) January 21, 2026 10:39

Joao-Dionisio merged commit 89d0391 into scipopt:master Jan 21, 2026
3 checks passed

Zeroto521 deleted the issue/1062 branch January 21, 2026 10:44

BUG: can't call getVal from GenExpr #1148

BUG: can't call getVal from GenExpr #1148

Uh oh!

Conversation

Zeroto521 commented Jan 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 9, 2026

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zeroto521 commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Joao-Dionisio commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zeroto521 commented Jan 21, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BUG: can't call `getVal` from `GenExpr` #1148

BUG: can't call `getVal` from `GenExpr` #1148

Zeroto521 commented Jan 19, 2026 •

edited

Loading

Joao-Dionisio commented Jan 20, 2026 •

edited

Loading