Feature: Merge Expr and GenExpr
#1114
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zeroto521
force-pushed
the
issue/1074
branch
from
c13b346 to
8719b91
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR merges the Expr and GenExpr classes into a unified expression system. The refactoring replaces the previous dual-system (polynomial Expr and general GenExpr) with a single hierarchy based on Expr, using specialized subclasses like PolynomialExpr, SumExpr, ProdExpr, and various function expressions (ExpExpr, LogExpr, etc.).
Key changes:
- Unified expression representation with
childrenreplacingterms Variableclass no longer inherits fromExpr- Simplified expression tree structure with improved type system
- Refactored constraint creation methods to use the new expression system
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pyscipopt/scip.pyi | Updated Variable class to remove inheritance from Expr |
| src/pyscipopt/scip.pxi | Modified Variable class structure and added operator overloading methods to delegate to expression system; refactored constraint creation methods to use children instead of terms |
| src/pyscipopt/scip.pxd | Updated Variable class declaration to remove Expr inheritance |
| src/pyscipopt/propagator.pxi | Simplified variable creation by removing unnecessary temporary variable |
| src/pyscipopt/expr.pxi | Complete rewrite of expression system replacing Expr/GenExpr duality with unified class hierarchy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Zeroto521
force-pushed
the
issue/1074
branch
from
3e0359d to
da49cca
Compare
Contributor
Author
|
Finish the base feature and function. And something needs to be done:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pyscipopt/expr.pxi
Outdated
| raise TypeError("expr must be an Expr instance") | ||
| if lhs is None and rhs is None: | ||
| raise ValueError( | ||
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" |
There was a problem hiding this comment.
The error message is grammatically incorrect. It should be "Ranged ExprCons (with both lhs and rhs) isn't supported" or "Ranged ExprCons (with both lhs and rhs) is not supported" instead of "doesn't supported".
Suggested change
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" | |
| "Ranged ExprCons (with both lhs and rhs) is not supported" |
|
|
||
|
|
||
| class ExprCons: | ||
| """Constraints with a polynomial expressions and lower/upper bounds.""" |
There was a problem hiding this comment.
The docstring says "Constraints with a polynomial expressions" but should say "Constraints with polynomial expressions" (without "a" before "polynomial"). Also note that ExprCons can now handle non-polynomial expressions too (like exp, log, etc.), so the docstring is inaccurate and should be updated to reflect that it handles general expressions.
Suggested change
| """Constraints with a polynomial expressions and lower/upper bounds.""" | |
| """Constraints with general expressions and lower/upper bounds.""" |
Member
|
Thanks for your work @Zeroto521 !! This is something that should be very very well tested before merging. We're also in the middle of a SCIP meeting, so I can bring this up with the guys and we can quickly take a high level look. |
Unified and improved the _to_node method logic in Term, Expr, PolynomialExpr, and UnaryExpr classes. The refactor centralizes node construction in Expr, removes redundant overrides, and clarifies argument order for consistency. This change simplifies expression tree construction for SCIP integration.
Short-circuit addition in Expr and PolynomialExpr classes when adding a zero constant, returning the original expression. This improves efficiency by avoiding unnecessary object creation.
Introduces a 'copy' parameter to Expr.to_dict for controlling whether children are copied. Refactors PolynomialExpr.__iadd__ to use to_dict with copy=False for efficiency when merging children.
Updated __hash__ implementations in ProdExpr, PowExpr, and UnaryExpr to include the class type, ensuring correct hashing for different expression types with similar contents.
Improves the __iadd__ method for Expr to handle sum expressions more efficiently by modifying in place, and adds an __isub__ method for in-place subtraction. This enhances performance and consistency when using += and -= operators with Expr objects.
Corrects the direction of comparison operations between Expr and MatrixExpr objects in __ge__ and __eq__ methods to ensure proper delegation and expected behavior.
`PolynomialExpr <= ConstExpr` will return `ExprCons(ConstExpr-PolynomialExpr, 0, one)` due to Subclass Priority
Applied consistent code formatting, improved readability with better indentation and line breaks, and updated string quoting. Refactored function and class definitions for clarity, and made minor variable naming and style adjustments throughout the test file.
Replaces the public attribute HASH with a private _hash attribute in the Term class to follow Python naming conventions and improve encapsulation.
Updated the degree method to use max(..., default=0) instead of returning float('inf') when there are no children, ensuring correct degree calculation for empty expressions.
Update the test to expect degree 0 for an empty expression instead of infinity, aligning with the intended behavior.
Refactored the value assignment logic in readStatistics to use a try-except block for float conversion, ensuring non-numeric values are handled gracefully. This makes the parsing more robust and eliminates reliance on isinstance checks.
Replaced direct type checks with static methods (_is_Sum and _is_Const) for improved consistency and maintainability. Updated arithmetic and comparison operations in Expr, PolynomialExpr, ConstExpr, and ProdExpr to use these new methods.
Moved the __neg__ method before __abs__ in the ConstExpr class for consistency and readability. No functional changes were made.
Introduced a _normalize method in the Variable class that returns the variable as a PolynomialExpr using Expr._from_var. This provides a standardized way to normalize variables.
Moved MatrixExprCons class definition above MatrixExpr and removed its duplicate definition at the end of the file. Also added import for Number from numbers to support type annotations.
Moved cdef readonly attributes for Expr and ExprCons from implementation to header file for better Cython type visibility. Added new cdef and cpdef methods to Expr and exposed quicksum and quickprod functions in scip.pxd. Also added necessary imports in matrix.pxi to support these changes.
Replaces integer op codes in _matrixexpr_richcmp with functions from the operator module for clarity and maintainability. Updates method signatures and internal logic to use operator.le, operator.ge, and operator.eq instead of magic numbers.
Moved the matrix comparison helper function into a static method MatrixExpr._cmp and updated MatrixExprCons and MatrixExpr to use it. This centralizes and simplifies the comparison logic for matrix expressions.
Introduces a custom __array_wrap__ method in MatrixExpr to handle type casting and result wrapping, replacing explicit operator overloads. This centralizes the logic for returning MatrixExpr or MatrixExprCons views, improving maintainability and consistency.
Introduced MatrixBase as a common base class for matrix expressions and variables, replacing direct usage of MatrixExpr in type checks and inheritance. Updated method signatures and isinstance checks to use MatrixBase, and expanded supported types for comparison and value retrieval methods to include Variable and MatrixVariable.
Moved the _cmp static method from MatrixBase to a module-level function to simplify and centralize comparison logic. Updated all internal calls to use the new _cmp function, improving code clarity and maintainability. Also removed outdated docstring comments.
Reworked MatrixExprCons and MatrixBase to use __array_ufunc__ for comparison operations, removed redundant __le__, __ge__, and __eq__ methods, and updated type hints to use string annotations for MatrixExpr. Also removed unused MatrixGenExpr class from type stubs and improved argument validation in Expr ufunc handling.
Updated TypeError and NotImplementedError messages to use consistent lowercase phrasing for improved readability and style consistency.
Enhanced type checking in constructors and methods by providing more informative error messages that include the actual type received. Replaced some 'all' checks with explicit for-loops for clearer error reporting. Minor corrections to exception messages and removed redundant code in UnaryExpr.
Introduces _ensure_unary to standardize input types for unary expressions, replacing scattered type checks. Updates all relevant methods and internal helpers to use _ensure_unary, improving code clarity and reducing duplication. Also renames _ensure_unary_compatible to _ensure_const for clarity.
Simplifies variable naming in Expr._to_dict and improves type handling. Updates PowExpr and UnaryExpr to ensure correct unwrapping and representation of child expressions, enhancing code clarity and correctness.
Added checks for None results from _to_expr in arithmetic and comparison methods of Expr and related classes, returning NotImplemented when the operand type is invalid. This improves robustness and ensures correct operator overloading behavior when unsupported types are used.
Changed the handling of Number types in _ensure_unary to wrap the result of _const in _ExprKey, ensuring consistent return types for expression construction.
Moved the expression equality logic from the Expr._is_equal method to a new standalone function _is_expr_equal. Updated all internal usages to call _is_expr_equal instead of the removed method. Cleaned up the Expr class and its interface accordingly.
Moved the _is_child_equal method from FuncExpr to a standalone cdef function for broader use. Updated ProdExpr and PowExpr to use the new function, improving code reuse and maintainability.
Replaces most Cython 'float' type annotations with 'double' in expression-related classes for improved type consistency and precision. Updates function signatures, class attributes, and internal helper functions accordingly. Also adds TYPE_CHECKING import guard for 'double' type hinting.
Replaces the hardcoded float('inf') in the degree method with a reusable INF constant. This improves code clarity and maintainability by centralizing the definition of infinity.
Removed the early return of NotImplemented for non-"__call__" methods and now delegate to the superclass implementation. This ensures correct behavior for all ufunc methods and improves compatibility with NumPy.
Refactored operator overloads in Expr and related classes to use explicit isinstance checks for Number, Variable, or Expr before conversion, returning NotImplemented for unsupported types. Added a TypeError in _to_expr for invalid input types to improve error reporting and robustness.
Changed the return type annotations of __richcmp__ and _cmp methods in Expr and related classes from ExprCons to object. This allows for greater flexibility in return types and improves compatibility with Python's comparison protocol.
Enhanced type checking in Expr._cmp to raise TypeError for unsupported types except numpy arrays. Added explicit __add__, __iadd__, __sub__, and __isub__ methods to ConstExpr for correct handling of constant expressions and type safety.
ensuring the provided 'cls' argument or the instance type is used for copying. This improves flexibility when copying Expr objects.
Enhanced arithmetic methods in Expr and related classes to handle zero values and type checks more robustly. Updated logic for addition, division, exponentiation, and polynomial/product/power expressions to ensure correct behavior when operands are zero or of specific types. Also replaced Expr._from_var with _var_to_expr for variable conversion.
Renamed loop variable from 'i' to 'arg' in the _ensure_array function for improved clarity and consistency.
Replaces the static method Expr._from_var with a new inline function _var_to_expr for converting Variable to PolynomialExpr. Updates all usages in Variable to use the new function, simplifying the code and improving clarity.
Moved the expression comparison logic from the Expr._cmp method to a new _expr_cmp helper function. Updated all relevant __richcmp__ methods in expression classes and Variable to use _expr_cmp, and removed the now-unused _cmp method from Expr. This improves code reuse and centralizes comparison logic.
Introduces _vec_const using numpy's frompyfunc to vectorize _const, and updates _ensure_const to handle numpy arrays and return MatrixExpr. Also updates type hints for exp, log, sqrt, sin, and cos to support MatrixExpr without quotes, improving consistency and type checking.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close to #1074. This is a breaking change. We can release the 5.7.0 version first.