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. |
Refactors the Variable.__iadd__ method to delegate in-place addition to MonomialExpr.from_var(self).__iadd__(other), ensuring correct behavior and consistency with other arithmetic operations.
Moved the static method _is_SumExpr to the end of the Expr class and updated its signature to use Cython type annotation. Also refactored variable naming in the __add__children method for clarity and removed unnecessary blank lines.
Implements __iadd__ for ConstExpr and MonomialExpr to support in-place addition with other polynomial expressions. Also refactors _first_child to _fchild and updates references for consistency.
Changed _is_SumExpr from a static method to an instance method in the Expr class. Updated all usages to call the method on instances, improving code clarity and consistency.
Updated type annotations for several methods to improve type safety and clarity, especially for functions handling MatrixExpr. Refactored AbsExpr construction and improved handling of special cases in PowExpr. Enhanced consistency in operator overloads and function signatures.
Updated the degree method in the Variable class to return a float instead of an int, aligning the return type with MonomialExpr.from_var(self).degree().
The Term class constructor now raises a TypeError if any argument is not a Variable instance, ensuring type safety and preventing incorrect usage.
Changed the return type annotation of the __iter__ method in the Expr class to Iterator[Union[Term, Expr]], removing Variable from the union for improved type accuracy.
Simplifies Expr initialization by removing Variable from children keys and direct MonomialExpr conversion. Refactors unary function constructors (exp, log, sqrt, sin, cos) to accept numbers and variables directly, using a unified to_subclass method in UnaryExpr. This improves API usability and code clarity.
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.
Replaces direct access to the _children attribute of expression objects with the public items() method throughout the Model class. This improves encapsulation and future-proofs the code against changes in the internal structure of expression objects.
Replaces direct Expr instantiation with the new _expr helper function for constructing Expr objects from dictionaries. This change improves consistency and encapsulation in expression creation.
Replaces direct instantiation of PolynomialExpr with a more generic _expr factory function that accepts the target class as an argument. This change improves code reuse and consistency in expression construction.
Replaces direct PowExpr and ProdExpr instantiation with helper functions _pow and _prod for consistency and encapsulation. Updates test to provide required exponent argument to PowExpr.
Removed the default value for the 'expo' parameter in PowExpr's constructor, requiring callers to always specify the exponent. Updated related test to provide the exponent explicitly.
Introduces tests for AbsExpr, SqrtExpr, CosExpr, and related expression classes in the context of the PySCIPOpt model. Tests cover initialization and the _to_node method to ensure correct behavior.
Introduces a new test file for ExprCons covering initialization errors, comparison operator error cases, boolean conversion, and string representation. Ensures robust error handling and correct behavior for edge cases in ExprCons.
Implements the __len__ method for the Term class to return the number of variables. Updates the degree() method to use __len__ for consistency.
Introduces an items() method to the Variable class, returning the degree of the variable via Expr._from_var(self).degree().
after iadd the const type is still const
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.
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.